Closed Bug 1503496 Opened 10 months ago Closed 10 months ago

Change JSScript flag bitfields to enum + flag words

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [overhead:20k])

Attachments

(1 file)

Using an uint64_t (or maybe better for alignment and easier for the JITs: two uint32_t fields) instead of the current C++ bitfields has a few benefits:

(1) XDR and CopyScript don't need code to copy/encode/decode each flag, we can just work with the flag words.

(2) It will be much easier to read script flags from JIT code.

(3) When adding a new flag it's easier to reason about sizeof(JSScript).
Depends on: 1502481
One thought I have here is to consider splitting into immutable and mutable flags.

In the back of my head I've wanted flags from CompileOptions to be packed into some flagword/bitfield and then copied directly to the JSScript. The intent would be to make writing a bytecode cache lookup much simpler for complex cases that come up in things like the Gecko startup cache.
(In reply to Ted Campbell [:tcampbell] from comment #1)
> One thought I have here is to consider splitting into immutable and mutable
> flags.

Most flags are one of (1) flags that are only set when the script is created and handled by XDR/CopyScript (2) flags that are set at runtime based on script behavior and ignored by XDR/CopyScript.

ImmutableFlags/MutableFlags might make sense for that.
This makes it easier to read these flags from JIT code. The patch also splits
them in MutableFlags and ImmutableFlags, this should let us simplify XDR and
CopyScript in the future.
Is there a good reason for using |1 << x| for enum values? My preference is to only assign specific enum values if they are required for correctness or perf. I'd rather boring code be boring if we can in this case.

(Immutable / Mutable split looks good!)
(In reply to Ted Campbell [:tcampbell] from comment #4)
> Is there a good reason for using |1 << x| for enum values? My preference is
> to only assign specific enum values if they are required for correctness or
> perf. I'd rather boring code be boring if we can in this case.

I use |1 << x| here because (1) it makes it easy to see how many flags are available when adding a new one and (2) it makes the code that uses these values (hasFlag/setFlag/clearFlag, code in jit/ in the future) less complicated because they don't have to do the |1 << x| thing.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cffce9a91124
Use enums + flag words for JSScript flags instead of bitfields. r=tcampbell
https://hg.mozilla.org/mozilla-central/rev/cffce9a91124
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
This seems to have regressed base content process JS memory usage by 20k. Is that expected, and if so is there anything we can do to pack things better?
Flags: needinfo?(jdemooij)
Whiteboard: [overhead:20k]
(In reply to Eric Rahm [:erahm] from comment #8)
> This seems to have regressed base content process JS memory usage by 20k. Is
> that expected, and if so is there anything we can do to pack things better?

As shown in perfherder: https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1684866,1,4&series=autoland,1684884,1,4&series=mozilla-inbound,1684802,1,4&zoom=1541470622441.3396,1541636058000,4522136.655023917,4592343.839863093&selected=autoland,1684884,400480,634697312,4
(In reply to Eric Rahm [:erahm] from comment #8)
> This seems to have regressed base content process JS memory usage by 20k. Is
> that expected, and if so is there anything we can do to pack things better?

Hm so sizeof(JSScript) didn't change here on OS X but it does when compiling with profiling/VTune support because of vtuneMethodId_ being #ifdef MOZ_VTUNE :/ That's silly, all scripts are paying the price for a feature almost nobody uses. I'll see if we can make that a HashMap.

FWIW there's some more low-hanging fruit in JSScript, I'll file bugs.
The patch in bug 1505690 should fix the sizeof(JSScript) regression.
Flags: needinfo?(jdemooij)
Blocks: 1505784
Blocks: 1507120
You need to log in before you can comment on or make changes to this bug.