Closed Bug 1571446 Opened 5 years ago Closed 5 years ago

Use a union for jitScript_ and warmUpCount_ fields in JSScript

Categories

(Core :: JavaScript Engine, task, P2)

Product:
Core
Component:
JavaScript Engine
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1571446 part 1 - Make JSScript::jitScript() assert hasJitScript() and add JSScript::maybeJitScript(). r?tcampbell!
(deleted), text/x-phabricator-request
Details
Bug 1571446 part 2 - Use accessors instead of accessing jitScript_ directly in various JSScript methods. r?tcampbell!
(deleted), text/x-phabricator-request
Details
Bug 1571446 part 3 - Combine JSScript's jitScript_ and warmUpCount_ fields in a single warmUpData_ field. r?tcampbell!
(deleted), text/x-phabricator-request
Details

The idea is that the warm-up count would be stored in JitScript, but until the script has a JitScript we can store small values directly in JSScript instead of in JitScript. The JITs and Baseline Interpreter rely on JitScript so they will always use the field in JitScript.

This is working towards the design in bug 1529456 comment 1.

Priority: -- → P2

Stupid remark.

How high can a warm-up counter reach while stored in the JSScript?
Maybe, we should stop incrementing the warm-up counter when running in interpreter-only mode.

(In reply to Nicolas B. Pierron [:nbp] from comment #1)

How high can a warm-up counter reach while stored in the JSScript?
Maybe, we should stop incrementing the warm-up counter when running in interpreter-only mode.

Yeah, I think we would limit it to 1000 or so, well within the page size (4096) and good enough for the C++ interpreter.

maybeJitScript() will become a bit slower in the future but many callers know
statically they have a script with a JitScript so by calling jitScript() there
we can avoid the hasJitScript() check in opt builds.

This is also consistent with the baselineScript() and ionScript() accessors.

I renamed jitScript() to jitScriptX() and then fixed all callers to call
the right method.

This makes it easier to change the jitScript_ field in later patches.

Depends on D42288

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Keywords: leave-open
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b2089a5f1393 part 1 - Make JSScript::jitScript() assert hasJitScript() and add JSScript::maybeJitScript(). r=tcampbell https://hg.mozilla.org/integration/autoland/rev/8f72a3e136c8 part 2 - Use accessors instead of accessing jitScript_ directly in various JSScript methods. r=tcampbell

The warm-up count is stored in ScriptWarmUpData until the script is warm
enough for the Baseline Interpreter and the JitScript is created. At that point
we use the warm-up count stored in JitScript.

ScriptWarmUpData uses pointer tagging. This should make it easy to add new
types for LazyScript data in the future.

Keywords: leave-open
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e753d23c1237 part 3 - Combine JSScript's jitScript_ and warmUpCount_ fields in a single warmUpData_ field. r=tcampbell
Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5921edc9bf59 follow-up - Don't use std::min to avoid weird linker errors in no-opt debug builds. CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/e753d23c1237
https://hg.mozilla.org/mozilla-central/rev/5921edc9bf59

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Attachment #9099833 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: