Closed Bug 1719194 Opened 3 years ago Closed 3 years ago

`FullParseHandler` depends on BaseScript pointer.

Categories

(Core :: JavaScript Engine, enhancement, P1)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
92 Branch
Tracking Flags:
Tracking Status
firefox92 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Bug 1719194 part 0 - Add a new TaggedScriptThing to be used as a placeholder.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719194 part 1 - Use a stencil-like gc-things vector for compiling.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719194 part 2 - Use stencil-like data inferred from BaseScript for skipping functions.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719194 part 3 - Remove Rooted<BaseScript*> from the FullParseHandler.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719194 undo part 0 - Remove OpaqueThingType added in part 0.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719194 part 4 - Move previous parse cache in the CompilationState.
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719194 part 5 - Reorganize the previous parse cache to hide internals.
48 bytes, text/x-phabricator-request
Details | Review

Follow-up of Bug 1718102, we have to replace uses of BaseScript iteration over gc-things to be able to extract the same information from the Stencil directly without having references to any allocated JSFunction.

https://searchfox.org/mozilla-central/rev/b7bc94b4689a7f002c61d016c6e162e5e5708bf3/js/src/frontend/Parser.cpp#2901-2964

In upcoming patches, we are converting the BaseScript::gcThings to a fake
stencil vector of TaggedScriptThingIndex. As we transition progressively, some
kind are not yet represented and therefore not used. To avoid encoding bad
indexes, we create a new OpaqueThing type which is meant to represent all things
we do not yet care about.

When delazifying with a CompileLazyFunction call, we reused
BaseScript::gcthings to skip over information already provided by the
SyntaxParseHandler, such as functions and closed-over bindings.

This change adds a stencil-like gcthings span, which is created from the
BaseScript::gcthings() with the intent of later using the same API for mapping
input data from a Stencil without having to allocate GC objects for eager
delazifying functions.

CompilationState::init is modified to call the convertion from the
BaseScript::gcthings() to the CompilationInput::cachedGcThings_. The logic of
the FullParseHandler is updated to work with the gcthings provided by the
CompilationInput in a similar way, except that atoms contained in the original
vector are internalized under CompilationState::init instead of being
internalized when visiting each scope under
propagateFreeNamesAndMarkClosedOverBindings.

When delazifying with a CompileLazyFunction call, we reused
BaseScript::gcthings to skip over information already provided by the
SyntaxParseHandler, such as functions and closed-over bindings.

This change adds a stencil-like scriptData and scriptExtra spans, which are
created from the JSFunctions of BaseScript::gcthings() with the intent of later
using the same API for mapping input data from a Stencil without having to
allocate GC objects for eager delazifying functions.

CompilationState::init is modified to call the convertion from the
BaseScript::gcthings() to the CompilationInput::cacheScriptData_ and
CompilationInput::cacheScriptExtra_. The logic of the FullParseHandler is
updated to work with the gcthings provided by the CompilationInput in a
similar way.

The function skipLazyInnerFunction now takes its inputs from the
ScriptStencil and from the ScriptStencilExtra, thus no longer relying on
BaseScript::gcthings() nor on a JSFunction* to extract this information
from. FunctionBox initialization reflect these changes by taking these 2
inputs as argument as well.

Note that the ScriptStencil and ScriptStencilExtra provided by the
CompilationInput do not reflect the full content of each script, but the bare
minimum used by skipLazyInnerFunction.

In order to be able to parse from an existing Stencil instead of a BaseScript*,
we have to replace all references to BaseScript* data by abstract data which can
be computed from both, and without the need for allocating GC objects.

This patch is the last change to the FullParseHandler to abstract over a GC
object (BaseScript*) and a Stencil. It removes all references to the
Rooted<BaseScript*> and replaces them by a simple boolean flag which indicate
whether we are reusing data from a pre-parsed function, instead of doing a full
parse which includes resolving bindings and parsing inner functions.

This patch also include a renaming of functions named canSkip* into reuse*,
to better highlight why we are doing differently.

The OpaqueThingType placeholder was necessary when making this series of
patches. It is no longer needed anymore.

Summary: `Parser<FullParseHandler, Unit>::skipLazyInnerFunction` depends on BaseScript pointer. → `FullParseHandler` depends on BaseScript pointer.

This patch moves data from the CompilationInput to the CompilationState such
that the lifetime of the allocation spans are matching the LifoAlloc scope
maintained by the CompilationState.

A new structure named CompilationSyntaxParseCache is added to hold gcThings,
scriptData and scriptExtra spans which are made to either be allocated or alias
previous Stencil produced by the SyntaxParseHandler.

The FullParseHandler, instead of copying each span, now reference the
CompilationSyntaxParseCache as a constant.

This patch does 2 things:

  • It moves the closedOverBindings out of the cachedGCThings_.
  • It hides the ScriptIndex extracted from cachedGCThings_.

The cachedGCThings_ used to contain both the TaggedParserAtomIndex and the
ScriptIndex as a TaggedScriptThingIndex. These 2 indexes are referring to data
which are not the same sources. The TaggedParserAtomIndex are indexes in the
parseAtom table, while the ScriptIndex are either newly allocated or reference
to an existing Stencil which is used as input.

Thus, the closedOverBindings_ are moved away of the cachedGCThings_ to highlight
that these are not indexes into an existing Stencil, but references to the
CompilationState.

As the ScriptIndex contained in the cachedGCThings_ are not valid for the
FullParseHandler, the ScriptIndex returned value is now hidden under private
functions, and the function index is used as a replacement. This way, the
ScriptIndex reference should not leak in the FullParseHandler, thus avoiding
confusion.

Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd6a7fdc412e
part 0 - Add a new TaggedScriptThing to be used as a placeholder. r=arai
https://hg.mozilla.org/integration/autoland/rev/6fbc8ee2c1bd
part 1 - Use a stencil-like gc-things vector for compiling. r=arai
https://hg.mozilla.org/integration/autoland/rev/74fe6205ace1
part 2 - Use stencil-like data inferred from BaseScript for skipping functions. r=arai
https://hg.mozilla.org/integration/autoland/rev/09725af9e55b
part 3 - Remove Rooted<BaseScript*> from the FullParseHandler. r=arai
https://hg.mozilla.org/integration/autoland/rev/a0b7896b840d
undo part 0 - Remove OpaqueThingType added in part 0. r=arai
https://hg.mozilla.org/integration/autoland/rev/5b1c02e1173b
part 4 - Move previous parse cache in the CompilationState. r=arai
https://hg.mozilla.org/integration/autoland/rev/d5966f092353
part 5 - Reorganize the previous parse cache to hide internals. r=arai
Blocks: 1723556
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4c094e5d627
part 0 - Add a new TaggedScriptThing to be used as a placeholder. r=arai
https://hg.mozilla.org/integration/autoland/rev/2e4004a83d66
part 1 - Use a stencil-like gc-things vector for compiling. r=arai
https://hg.mozilla.org/integration/autoland/rev/cd0c6fe11a43
part 2 - Use stencil-like data inferred from BaseScript for skipping functions. r=arai
https://hg.mozilla.org/integration/autoland/rev/7a3f8f2bbe43
part 3 - Remove Rooted<BaseScript*> from the FullParseHandler. r=arai
https://hg.mozilla.org/integration/autoland/rev/5beb66bb84f4
undo part 0 - Remove OpaqueThingType added in part 0. r=arai
https://hg.mozilla.org/integration/autoland/rev/e1a35ec2f610
part 4 - Move previous parse cache in the CompilationState. r=arai
https://hg.mozilla.org/integration/autoland/rev/70103d58923c
part 5 - Reorganize the previous parse cache to hide internals. r=arai
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eef525ed6fc1
part 0 - Add a new TaggedScriptThing to be used as a placeholder. r=arai
https://hg.mozilla.org/integration/autoland/rev/476682f54e5c
part 1 - Use a stencil-like gc-things vector for compiling. r=arai
https://hg.mozilla.org/integration/autoland/rev/c35855c033f8
part 2 - Use stencil-like data inferred from BaseScript for skipping functions. r=arai
https://hg.mozilla.org/integration/autoland/rev/9af4c3626c57
part 3 - Remove Rooted<BaseScript*> from the FullParseHandler. r=arai
https://hg.mozilla.org/integration/autoland/rev/5111a4144510
undo part 0 - Remove OpaqueThingType added in part 0. r=arai
https://hg.mozilla.org/integration/autoland/rev/79f43b750dd8
part 4 - Move previous parse cache in the CompilationState. r=arai
https://hg.mozilla.org/integration/autoland/rev/dc6a883c1090
part 5 - Reorganize the previous parse cache to hide internals. r=arai
Flags: needinfo?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: