Bug 128050 – Keep only captured symbols in CodeBlock symbol tables.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andreas Kling]

Keep only captured symbols in CodeBlock symbol tables.

[Andreas Kling]

Created attachment 222890 [details]
Patch

[Geoffrey Garen]

Comment on attachment 222890 [details]
Patch

It looks like the code already dropped uncaptured symbols, and you've changed its approach to reuse an existing table and remove items, rather than create a new table and selectively insert items.

Have I read that right?

What's the motivation for that change? Is there some other object that points to the BytecodeGenrator's SymbolTable? Or do you think removal will be more efficient than creating a copy or something?

(If another object also points to the BytecodeGenrator's symbol table, I think a clearer solution might be to separate that object, and force it to clone the SymbolTable as well.)

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 222890 [details])
> It looks like the code already dropped uncaptured symbols, and you've changed its approach to reuse an existing table and remove items, rather than create a new table and selectively insert items.
> 
> Have I read that right?
> 
> What's the motivation for that change? Is there some other object that points to the BytecodeGenrator's SymbolTable? Or do you think removal will be more efficient than creating a copy or something?
> 
> (If another object also points to the BytecodeGenrator's symbol table, I think a clearer solution might be to separate that object, and force it to clone the SymbolTable as well.)

The purpose of this change is to minimize the amount of memory used by SymbolTables hanging off of UnlinkedCodeBlocks. We currently keep the full symbol tables around, despite only ever accessing the captured symbols.

FWIW the reason we clone the SymbolTable in the first place is to avoid watchpoint reuse.

[Andreas Kling]

How about this:

* Rename CodeBlock::symbolTable() into CodeBlock::capturedSymbolTable().
* Fork off a CapturedSymbolTable class from SymbolTable.
* CapturedSymbolTable inherits from UnconditionalFinalizer.
* WatchpointCleanup goes away (IIUC the JSGlobalObject symbol table doesn't need this mechanism.)
* Fork off a CapturedSymbolTableEntry class from SymbolTableEntry.
* CapturedSymbolTableEntry always has a watchpoint set. SymbolTableEntry never has a watchpoint set.
* Slim/fat logic in both classes disappears.

[Geoffrey Garen]

Comment on attachment 222890 [details]
Patch

r=me because this patch is better than the status quo.

I think it would be even better to make a new SymbolTable, and just copy the captured things into it. (In the common case, nothing will be captured, so that will be faster.)

[Geoffrey Garen]

(In reply to comment #4)
> How about this:
> 
> * Rename CodeBlock::symbolTable() into CodeBlock::capturedSymbolTable().
> * Fork off a CapturedSymbolTable class from SymbolTable.
> * CapturedSymbolTable inherits from UnconditionalFinalizer.
> * WatchpointCleanup goes away (IIUC the JSGlobalObject symbol table doesn't need this mechanism.)
> * Fork off a CapturedSymbolTableEntry class from SymbolTableEntry.
> * CapturedSymbolTableEntry always has a watchpoint set. SymbolTableEntry never has a watchpoint set.
> * Slim/fat logic in both classes disappears.

I think this is right, but I think the JSGlobalObject symbol table will still need to be the version of symbol table that has an UnconditionalFinalizer.

[Geoffrey Garen]

I also think it would be nice for the BytecodeGenerator to stop sharing its SymbolTable* with some other object. Instead, BytecodeGenerator should create its own symbol table, and perform an explicit copy if any other object needs that data.

[Andreas Kling]

Committed r163337: <http://trac.webkit.org/changeset/163337>