1464036 - Ensure JSID_VOID is represented as 0

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jan de Mooij [:jandem]]

Attachment: Some jsid changes

Per discussion in bug 1460341, this makes the following changes:

* Switches JSID_TYPE_VOID and JSID_TYPE_STRING. This means atom <-> id conversions now need an extra bitwise op, but I think that's acceptable. The alternative is to represent JSID_VOID as a nullptr string, but then JSID_IS_STRING needs an extra branch.

* JSID_TYPE_EMPTY now has its own type instead of being represented as a nullptr symbol. This eliminates a branch from JSID_IS_SYMBOL.

jsid really needs to be refactored to use modern C++ and proper encapsulation like JS:Value; we had a bunch of code assuming JSID_TYPE_STRING is 0 :(

Anyway, comparing libxul.so for a Linux64 Opt build on Try to the base revision on inbound:

- before:  130,515,432 bytes
- after:   130,347,952 bytes 

So about 163 KB smaller. In bug 1460341, erahm mentioned a 144 KB regression. I think the improvement is bigger than that because we also have many PinnedStringIds and these were also initialized to JSID_VOID before bug 1460341.

Comment 1

[Jan de Mooij [:jandem]]

I'll also compare libxul sizes on OS X and Windows to make sure regressions there (if any) are fixed by this as well.

Comment 2

[Jan de Mooij [:jandem]]

OS X, for "XUL": 

Before: 168,096,880
After:  167,952,824

So that's 140 KB and sounds about right.

Win64 opt:

Revision before bug 1460341: 80791552
Revision after bug 1460341:  80841728
Inbound:                     81400832
Inbound + patch:             81396736

So we had a 49 KB regression when that landed, the patch wins 4 KB. However I'm not sure how reliable this data is considering the 550 KB increase in less than 2 weeks.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> This means atom <-> id conversions now need an extra bitwise op

Ah, that's why string was 0!

Comment 4

[Jan de Mooij [:jandem]]

I was able to confirm with dmajor's WinDbg method that we indeed have dynamic initializers for all the property infos in bindings code, both with and without the patch here :/

A patch to leave these particular ids uninitialized seems to fix it. So I'll (1) put together a patch for the bindings code to not initialize these ids at all and (2) land the patch here, but with the VOID and STRING tags unchanged (compared to current m-c).

Comment 5

[Boris Zbarsky [:bzbarsky]]

I thought we had tests for number of static initializers.  Are those not catching the dynamic initializers in question?  :(

Comment 6

[(Away)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> I thought we had tests for number of static initializers.  Are those not
> catching the dynamic initializers in question?  :(

IIRC those run on Linux builds, right?

Comment 7

[Boris Zbarsky [:bzbarsky]]

That's possible...  I wonder whether we can add them on Windows too.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Remove PropertyInfo constructor

This just replaces the jsid in PropertyInfo with |uintptr_t mIdBits| and adds accessors to convert to/from jsid.

I verified I no longer see PropertyInfo initializers for Win64 opt builds with this patch.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Remove PropertyInfo constructor

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8980668 [details] [diff] [review]
Remove PropertyInfo constructor

>+  jsid Id() const {

Should probably be ALWAYS_INLINE?

Are we guaranteed that fromRawBits gets inlined?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> Should probably be ALWAYS_INLINE?
> 
> Are we guaranteed that fromRawBits gets inlined?

I'll add MOZ_ALWAYS_INLINE to Id() and fromRawBits. Both are basically no-ops when inlined.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Both are basically no-ops when inlined.

Right, that's the idea.  ;)

Comment 13

[Mike Hommey [:glandium]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> That's possible...  I wonder whether we can add them on Windows too.

Yes, we can! And relatively simply, at that. Filed bug 1464522

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7381b7b49ef2
Remove PropertyInfo constructor to keep MSVC from generating static initializers. r=bz

Comment 15

[Jan de Mooij [:jandem]]

I still want to land a modified version of the other patch too, because it has some nice cleanup.

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7381b7b49ef2

Comment 17

[Mike Hommey [:glandium]]

FWIW, this is what the number of static initializers looked like:
             bug 1460341       this bug
            before  after   before   after
win32-opt:    515    1304    1299     516
win32-pgo:    472    1261    1257     474
win64-opt:    570    1359    1357     574
win64-pgo:    559    1304    1347     564

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Give JSID_EMPTY its own jsid tag and clean up jsid code a bit

This is the patch you already reviewed, except I changed JSID_TYPE_STRING back to 0 and JSID_TYPE_VOID back to 2 because the other patch got rid of the PropertyInfo static initializers.

The patch still gives JSID_EMPTY its own type tag and uses JSID_TYPE_STRING instead of relying on it being 0 (compilers will just optimize it away and it will make it easier to change JSID_TYPE_STRING later if we want to).

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a276f4e1d09
part 2 - Give JSID_EMPTY its own jsid tag and clean up jsid code a bit. r=jonco

Comment 20

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6a276f4e1d09