Merge TypeScript and ICScript into JitScript

RESOLVED FIXED in Firefox 69

Status

()

task
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 2 bugs)

unspecified
mozilla69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox69 fixed)

Details

Attachments

(11 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

The plan is to do the following:

  1. Rename TypeScript to JitScript.
  2. Move it from vm/TypeInference.* to jit/JitScript.{h,cpp}
  3. Merge ICScript into JitScript. TypeScript and ICScript have the same lifetime already so this should be straight-forward. It will eliminate a malloc/free + extra dereferences + we can do a single bytecode walk instead of two when allocating JitScript.
  4. Move JIT-related fields currently in JSScript (baseline/ion/etc) to JitScript. This is nice because sizeof(JSScript) matters and it will move some JIT-related code out of vm/
Type: defect → task
Depends on: 1552470

Also JSScript::makeTypes => JSScript::createJitScript for consistency with code
elsewhere.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED

Some of the TypeInference-related static methods were kept in TypeInference-inl.h and
TypeInference.cpp because of various dependencies. Also, methods like JitScript::MonitorAssign
probably don't belong on JitScript anyway, but for now this seems reasonable.

Depends on D31755

ICScript and JitScript had the same lifetime already, but this eliminates
a malloc/free and some extra dereferences (especially for accessing ICEntries
from Baseline Interpreter code).

This is just the minimal set of changes to make it easier to review. Follow-up
changes should:

  • Move (now) JitScript methods to JitScript.cpp

  • Merge FillBytecodeTypeMap with JitScript::initICEntries so we do just a single
    bytecode walk.

  • Move JitScript from the js namespace to js::jit.

Depends on D31982

These have been unnecessary since the ICScript introduction: when performing the
arguments analysis we have a JitScript but not a BaselineScript.

Depends on D32103

The destroy() call in JSScript::finalize was moved into DestroyJitScripts for
consistency with BaselineScript and IonScript.

"Discard" instead of "Release" for consistency with ShouldDiscardBaselineCode.

Depends on D32293

We now do a single pass over the bytecode instead of two.

Register pressure of the combined code might be a bit worse but it also
eliminates some duplication so I think it's worth it.

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ac16f08196a
part 1 - Rename TypeScript to JitScript. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/330f42984cf4
part 2 - Move JitScript from vm/TypeInference.* to jit/JitScript.*. r=tcampbell
Keywords: leave-open
Duplicate of this bug: 1511739

JitScript::initICEntriesAndBytecodeTypeMap is still in BaselineIC.cpp because
it depends on things defined there (like FallbackStubAllocator) and I think it's
not unreasonable to keep it there.

Depends on D32299

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e81b72b58694
part 3 - Merge ICScript into JitScript. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/71c4f7fd04f0
part 4 - Remove unnecessary hasJitScript() checks in BaselineInspector. r=tcampbell
Priority: -- → P1
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0f95fe29f8a
part 5 - Make JitScript::destroy static. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/547d1940a27f
part 6 - Use DefaultInitializeElements to initialize JitScript's StackTypeSet array. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/56f1c5f12b84
part 7 - Improve the JitScript comment a bit. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/5ee66c20f298
part 8 - Rename ShouldReleaseTypes to ShouldDiscardJitScripts. r=tcampbell
Blocks: 1554080
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05c094ef90ca
part 9 - Merge FillBytecodeTypeMap into JitScript::initICEntries. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/87aaa29b2fff
part 10 - Move JitScript to js::jit namespace. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/ec5f175f4f2c
part 11 - Move more JitScript code into JitScript.cpp. r=tcampbell
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.