fix(script): drop AST after script execution#4876
Conversation
`Script::Inner` held `source: boa_ast::Script` forever. Every function object created during execution stores `OrdinaryFunction::script_or_module`, which is a strong `Gc` reference back to `Script::Inner`. For global functions this keeps the full parsed AST in memory indefinitely. Change the field to `RefCell<Option<boa_ast::Script>>` and set it to `None` at the end of `codeblock()`, mirroring the same fix already applied to modules after linking. Signed-off-by: ashnaaseth2325-oss <ashnaaseth2325@gmail.com>
Test262 conformance changes
Tested main commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4876 +/- ##
===========================================
+ Coverage 47.24% 57.45% +10.21%
===========================================
Files 476 556 +80
Lines 46892 60714 +13822
===========================================
+ Hits 22154 34886 +12732
- Misses 24738 25828 +1090 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hello @zhuzhu81998 |
|
Hello @hansl @jedel1043 |
| source: boa_ast::Script, | ||
| source: RefCell<Option<boa_ast::Script>>, | ||
| source_text: SourceText, | ||
| codeblock: GcRefCell<Option<Gc<CodeBlock>>>, |
There was a problem hiding this comment.
I think a better approach would be to make this item an enum like
enum ScriptPhase {
Ast(boa_ast::Script),
Codeblock(Gc<CodeBlock>)
}Then we transition through the phases of compilation of the script in codeblock.
This saves some nice bytes of memory representation.
|
Hello @jedel1043 |
| if let ScriptPhase::Codeblock(codeblock) = &*phase { | ||
| return Ok(codeblock.clone()); | ||
| } |
There was a problem hiding this comment.
Let's make use of Rust features :)
let phase = self.inner.phase.borrow();
let source = match &*phase {
ScriptPhase::Codeblock(codeblock) => return Ok(codeblock.clone()),
ScriptPhase::Ast(source) => source
}There was a problem hiding this comment.
I edited this btw, at first I used a clone but I realized that would clone the whole AST, which we don't want
|
Hello @jedel1043 |
Context
This PR extends the idea introduced in #4855.
PR #4855 fixed a memory retention issue for modules, where the parsed AST and source text were kept in memory even after module initialization.
This PR applies the same idea to scripts, ensuring that the parsed AST is released once script execution has completed.
Summary
This PR prevents scripts from keeping their parsed AST after compilation.
Right now
Script::Innerstores the full AST in thesourcefield:The AST is only needed while generating bytecode in
Script::codeblock().After compilation it is not used anymore, but it still stays in
Script::Inner.Function objects created from the script keep a reference to the script (
OrdinaryFunction::script_or_module). Because of this, the script can stay alive for a long time, which also keeps the whole AST in memory.Steps to Reproduce
In a long-running program (for example a REPL or server), repeatedly run scripts that define functions:
If those functions are stored on the global object, they keep the script alive.
Since the script holds the AST, memory usage keeps growing as more scripts run.
Impact
This mostly affects long-running programs that embed Boa.
Every script that defines functions can keep its AST in memory forever.
AST structures are usually much bigger than the source code, so memory usage can grow over time.
Modules already drop their AST after linking, but scripts did not.
Fix
Script::Inner::sourceis changed to:The AST is used during compilation and then removed once the
CodeBlockis created:Result
After this change, the AST is dropped once compilation finishes.
Scripts now behave consistently with modules after the change introduced in #4855.
The AST is only required during parsing and compilation. Once the script finishes execution and the result is consumed, it can be safely released to reduce memory usage in long-running runtimes.