Closed Bug 1280648 Opened 4 years ago Closed 4 years ago

Tracelogger: don't cache based on pointer to CompilerOptions

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(1 file)

CompilerOptions pointer is not unique to the data we want to store. As a result we cannot use the pointer as an id for the cache. For now just allow to not cache it.
Attached patch PatchSplinter Review
1) add a way to insert a textId to text without a key to cache the data
2) Don't cache the CompileOptions anymore, since the pointers are not unique!

Note: I'm not entirely sure, but if a JSScript* can move we will also need to opt-out and create follow-up to clear the cache every time we move JSScripts. Not sure about the status. We have a Handle to JSScript, but also use raw JSScript pointers?
Assignee: nobody → hv1989
Flags: needinfo?(terrence)
Attachment #8763249 - Flags: review?(bbouvier)
Sfink indicated that JSScript* is not movable and will be one of the last thing to get movable. So we are currently still safe. There is now also HashMaps that support the moving. We can possibly update our cache with that eventually.
Flags: needinfo?(terrence)
Attachment #8763249 - Flags: review?(bbouvier) → review+
(In reply to Hannes Verschore [:h4writer] from comment #2)
> Sfink indicated that JSScript* is not movable and will be one of the last
> thing to get movable. So we are currently still safe. There is now also
> HashMaps that support the moving. We can possibly update our cache with that
> eventually.

I don't think that's true - compacting GC can move scripts. See bug 1259180.
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a942a270777
Tracelogger: Don't cache based on pointers to movable gc things, r=bbouvier
(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Hannes Verschore [:h4writer] from comment #2)
> > Sfink indicated that JSScript* is not movable and will be one of the last
> > thing to get movable. So we are currently still safe. There is now also
> > HashMaps that support the moving. We can possibly update our cache with that
> > eventually.
> 
> I don't think that's true - compacting GC can move scripts. See bug 1259180.

I also removed the caching for JSScript and opened bug 1281156 for using the cache again for JSScript.
https://hg.mozilla.org/mozilla-central/rev/7a942a270777
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1282383
Depends on: 1307354
You need to log in before you can comment on or make changes to this bug.