Tracelogger: don't cache based on pointer to CompilerOptions

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8763249 [details] [diff] [review]
Patch

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)
(Assignee)

Comment 2

2 years ago
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.

Comment 4

2 years ago
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
(Assignee)

Comment 5

2 years ago
(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.

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7a942a270777
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
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.