perf(bytecode): elide arguments object instructions when its never used #686
Conversation
|
issue: I don't think you can take the Semantic out: a single source code can contain X amount of functions, and they compile separately. |
| @@ -123,13 +126,15 @@ impl<'a> SourceCode<'a> { | |||
| // TODO: Include error messages in the exception. | |||
| return Err(errors); | |||
| } | |||
| let semantic = unsafe { core::mem::transmute::<Semantic, Semantic<'static>>(semantic) }; | |||
There was a problem hiding this comment.
issue: Safety comment needed. You can just extend the below unsafe {} block transmuting Program to encompass this as well, and modify the safety comment to cover both Program and Semantic.
| allocator, | ||
| }); | ||
| source.unbind(), |
There was a problem hiding this comment.
nitpick: You can probably get rid of this unbind by tweaking the new function.
Best possible scenario is that we have SourceCodeHeapData::new(source: HeapString<'a>) -> SourceCodeHeapData<'a> and then the CreateHeapData for SourceCodeHeapData<'a> would return SourceCode<'a>. That way the GC lifetime automatically gets threaded through here.
| @@ -150,9 +170,33 @@ pub struct SourceCodeHeapData<'a> { | |||
| /// string was small-string optimised and on the stack, then those | |||
| /// references would necessarily and definitely be invalid. | |||
| source: HeapString<'a>, | |||
| /// Semantic analysis results obtained when parsing a source file. | |||
| /// | |||
| /// This only exists until this source code gets compiled. Since it's only | |||
There was a problem hiding this comment.
issue: As mentioned in the one-off comment, parts of the source code get compiled at different times; each function instantiation (first time anyway) performs a new compilation. So, you cannot take the semantic out of here, at least not until you're sure that all the functions have been compiled.
But even then, maybe some day in the future we'll want to have tiered compilation where having the semantics available when we recompile a function is necessary. So, I think it's better to just leave the Semantic here.
| @@ -170,6 +214,10 @@ impl Drop for SourceCodeHeapData<'_> { | |||
| // SAFETY: All references to this SourceCode should have been dropped | |||
| // before we drop this. | |||
| drop(unsafe { Box::from_raw(self.allocator.as_mut()) }); | |||
| // drop( | |||
There was a problem hiding this comment.
note: The drop order should be "sema first, allocator second".
| source, | ||
| allocator: _, | ||
| } = self; | ||
| let Self { source, .. } = self; |
There was a problem hiding this comment.
issue: Do not use .. in the GC HeapMarkAndSweep trait. The explicit skips are intended; when we add fields to the struct, the .. makes it less likely that we'll remember to properly handle their GC.
eg. I've repeatedly forgot to add tracing of new fields back when I didn't yet have this rule of no .. in GC, which would then lead to weird bugs.
What This PR Does
argumentsobject when that function's body never references it