search

mozilla-spidermonkey / jsparagus

Experimental JS parser-generator project.
Other
447 stars 20 forks source link

Store atoms in gc things list (fixes #527) #528

Closed arai-a closed 4 years ago

arai-a commented 4 years ago

This follows https://bugzilla.mozilla.org/show_bug.cgi?id=1638470 change.

Now atoms are stored into gcthings list, instead of atom list per script. we still have a list of atoms, that is used by all scripts, and gcthings stores an index into it.

moztcampbell commented 4 years ago

Note that in the C++ parser we still deduplicate atom pointers before adding them to the gcthings list. (eg. This code remains https://searchfox.org/mozilla-central/rev/09b8072a543c145de2dc9bb76eddddd4a6c09adc/js/src/frontend/BytecodeEmitter.h#255-259 ).

Everything works correctly without this, but it is a useful memory optimization.

arai-a commented 4 years ago

Note that in the C++ parser we still deduplicate atom pointers before adding them to the gcthings list. (eg. This code remains https://searchfox.org/mozilla-central/rev/09b8072a543c145de2dc9bb76eddddd4a6c09adc/js/src/frontend/BytecodeEmitter.h#255-259 ).

Everything works correctly without this, but it is a useful memory optimization.

yes, deduplication is done in lexer, and AST holds an index into the unique list of atoms, for entire source. gcthings list holds that index, so the result is also unique.

smoosh part is responsible for converting the index into atom pointer. try run is here https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=9af63ccb660784e3b9e4e690f25acfcc79f19c44

arai-a commented 4 years ago

oh, just understand what you meant. yes, the patch is wrong. will fix shortly.