Move IC data out of BaselineScript

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P2
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Depends on 2 bugs, Blocks 2 bugs)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 wontfix, firefox65 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

7 months ago
ICEntries and the fallback stub space could be moved into a separate class that's stored in JSScript.

This is necessary for bug 1499324, but it will also allow us to simplify some BaseilneDebugModeOSR code (in particular, the DebugModeOSRVolatileStub complexity can go away I think) and that makes this worth doing now IMO.
Assignee

Updated

7 months ago
Depends on: 1499649
Assignee

Comment 1

7 months ago
(In reply to Jan de Mooij [:jandem] from comment #0)
> This is necessary for bug 1499324, but it will also allow us to simplify
> some BaseilneDebugModeOSR code (in particular, the DebugModeOSRVolatileStub
> complexity can go away I think)

Also the IC stub cloning we will no longer need! That's a bit less boilerplate with CacheIR, but it's still code we should be able to remove.
Assignee

Comment 2

7 months ago
Posted patch WIP (obsolete) — Splinter Review
This is a quick-and-dirty WIP patch:

* Adds ICScript storing the fallback stub space and the ICEntries.

* Removes > 230 lines of code from BaselineDebugModeOSR.* (and also related code from CacheIR and BaselineIC.cpp) because we no longer need to clone stubs.

* Passes almost all jit-tests, except for one known issue: ICTableSwitch needs to be rewritten (bug 1501316), the patch tries to workaround it but it doesn't work when we compile multiple BaselineScripts for one ICScript.

* The patch also never destroys ICScript once it's created. Shouln't be hard to fix.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Assignee

Updated

7 months ago
Depends on: 1501316
Assignee

Comment 3

7 months ago
Posted patch WIP (obsolete) — Splinter Review
(See the previous comment.)

Ted, do you mind glancing at this for a minute or two, to see if the approach makes sense to you? There are a bunch of known issues/nits, I'm just requesting feedback for the overall design.
Attachment #9019680 - Flags: feedback?(tcampbell)
Assignee

Updated

7 months ago
Attachment #9019669 - Attachment is obsolete: true
Assignee

Comment 4

7 months ago
diffstat, FWIW:

 19 files changed, 660 insertions(+), 869 deletions(-)
Comment on attachment 9019680 [details] [diff] [review]
WIP

Review of attachment 9019680 [details] [diff] [review]:
-----------------------------------------------------------------

ICScript seems like the right approach. I think this direction is worth continuing.

Leaving f? so I remember to give it more thought about details this week.

::: js/src/jit/BaselineIC.cpp
@@ +123,5 @@
>      }
>  }
>  
> +/* static */ bool
> +ICScript::create(JSContext* cx, JSScript* script)

We need a better way to correlate these. Some sort of JOF_* or equivalent would be nice so we can better assert on all side.

::: js/src/vm/JSScript.h
@@ +1415,5 @@
>    private:
>      /* Persistent type information retained across GCs. */
>      js::TypeScript* types_ = nullptr;
>  
> +    js::jit::ICScript* icScript_ = nullptr;

Unifying this with TypeScript one day would be nice. We probably have similar criteria for deciding this script is worth tracking types and adding ICs too. It also tells a nice story for Ion inspection.
Assignee

Comment 6

7 months ago
(In reply to Ted Campbell [:tcampbell] from comment #5)
> We need a better way to correlate these. Some sort of JOF_* or equivalent
> would be nice so we can better assert on all side.

Yeah, I think JOF_IC would be really nice to have (and we probably need it anyway, longer term). That said, the current patch will complain loudly if either (a) you add an IC to ICScript but don't use it in BaselineCompiler, or (b) you *don't* add an IC but expect one in BaselineCompiler. So I'd prefer adding JOF_IC in a follow-up bug (right after this lands) to not blow this patch up more.

> Unifying this with TypeScript one day would be nice. We probably have
> similar criteria for deciding this script is worth tracking types and adding
> ICs too. It also tells a nice story for Ion inspection.

Long-term I want to consider removing TypeScript completely and storing the StackTypeSets directly in the type monitor stubs (that will also save some memory because we allocate type monitor stubs lazily for JOF_TYPESET ops!). After that we could also remove StackTypeSet completely and having Ion build its TemporaryTypeSets directly from the attached type monitor stubs :)
(In reply to Jan de Mooij [:jandem] from comment #6) 
> Long-term I want to consider removing TypeScript completely and storing the
> StackTypeSets directly in the type monitor stubs (that will also save some
> memory because we allocate type monitor stubs lazily for JOF_TYPESET ops!).
> After that we could also remove StackTypeSet completely and having Ion build
> its TemporaryTypeSets directly from the attached type monitor stubs :)

A+
Assignee

Comment 8

7 months ago
Comment on attachment 9019680 [details] [diff] [review]
WIP

Review of attachment 9019680 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/x64/SharedICHelpers-x64.h
@@ +35,3 @@
>  {
> +    // Move ICEntry* into ICStubReg.
> +    masm.movePtr(ImmPtr(entry), ICStubReg);

Note to self: now that ICEntry* is a constant known at compile time we could probably use loadPtr + AbsoluteAddress... Doesn't matter much on x64 but on platforms like x86 it can eliminate an instruction.
Priority: -- → P2
QA Whiteboard: [qf:p1]
Assignee

Updated

7 months ago
Depends on: 1503170
Assignee

Updated

7 months ago
Blocks: 1501310
Assignee

Updated

7 months ago
Depends on: 1503522
Assignee

Comment 9

7 months ago
Comment on attachment 9019680 [details] [diff] [review]
WIP

I'll try to rebase and finish this soon. The updated patch should be nicer/simpler on top of the patches in the dependent bugs.
Attachment #9019680 - Flags: feedback?(tcampbell)
Assignee

Comment 10

6 months ago
Posted patch WIP v2 (obsolete) — Splinter Review
Rebased + some cleanup.

On top of all the changes that landed in the dependent bugs this now passes all jit-tests with --baseline-eager.

This needs a bunch of work still before we can land it, but it's moving in the right direction.
Attachment #9019680 - Attachment is obsolete: true
Assignee

Updated

6 months ago
Depends on: 1506479
Assignee

Updated

6 months ago
Depends on: 1506554
Assignee

Comment 11

6 months ago
Posted patch WIP v3 (obsolete) — Splinter Review
This version should be green on Try on top of the patches in the dependent bugs.

Now we just need some minor clean up work but everything should be in place.
Attachment #9023875 - Attachment is obsolete: true
Assignee

Updated

6 months ago
Depends on: 1506774
Assignee

Comment 12

6 months ago
ICEntries and the fallback stub space are now stored in ICScript. The ICScript*
is stored in TypeScript to not increase sizeof(JSScript).

We need this for bug 1499324 but it also lets us greatly simplify the
BaselineDebugModeOSR code as this patch shows.

Note: some ICScript method definitions are still in BaselineJIT.cpp instead of
BaselineIC.cpp to make this patch easier to review. We could move them to
BaselineIC.cpp as a follow-up change.
Assignee

Updated

6 months ago
Attachment #9024406 - Attachment is obsolete: true
Assignee

Comment 13

6 months ago
Ted, if you want to play with this locally, here's a Try push that has the most recent version except for some comments I added later: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=5290d02778215bf80b44cca98eadf5bdc1c7d094

Also this still removes ~250 lines of code:

 30 files changed, 721 insertions(+), 962 deletions(-)

The patch is big but a lot of it is very mechanical changes.
Assignee

Updated

6 months ago
Blocks: 1507066
Assignee

Updated

6 months ago
Blocks: 1508962

Comment 14

6 months ago
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6453222232be
Move IC data out of BaselineScript. r=tcampbell

Comment 15

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6453222232be
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1511415
Depends on: 1511417
Depends on: 1511412
Assignee

Updated

6 months ago
Depends on: 1511210
Assignee

Updated

6 months ago
Blocks: 1511739
You need to log in before you can comment on or make changes to this bug.