1503496 - Change JSScript flag bitfields to enum + flag words

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

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).

Comment 1

[Ted Campbell [:tcampbell]]

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.

Comment 2

[Jan de Mooij [:jandem]]

(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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1503496 - Use enums + flag words for JSScript flags instead of bitfields. r?tcampbell

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.

Comment 4

[Ted Campbell [:tcampbell]]

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!)

Comment 5

[Jan de Mooij [:jandem]]

(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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cffce9a91124
Use enums + flag words for JSScript flags instead of bitfields. r=tcampbell

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cffce9a91124

Comment 8

[Eric Rahm [:erahm]]

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?

Comment 9

[Eric Rahm [:erahm]]

(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

Comment 10

[Jan de Mooij [:jandem]]

(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.

Comment 11

[Jan de Mooij [:jandem]]

The patch in bug 1505690 should fix the sizeof(JSScript) regression.