630738 - JM: distinguish "get", "set" and "other" PICs to save space

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Spun off from bug 629601 comment 2:  "get" PICs have some specific info, "set" PICs have some specific info, "other" ("bind" and "name") PICs have no specific info.  If we sub-type them we can save some space in "get" and "other" PICs:

- 32-bit:  64 B --> 56/64/48 B
- 64-bit: 112 B --> 104/112/96 B

ie. the size of "set" PICs will be unchanged, "get" PICs (the most common) will shrink by 8 bytes, "other" PICS will shrink by 16 bytes.

Tracking three kinds of PICs instead of one in JITScript will cost an extra 8 bytes once bug 630445 is done.

Because PICs are much more common than JITScripts, this will be a net win.

Comment 1

[Nicholas Nethercote [inactive]]

Also, if 26 bits is enough for typeCheckOffset, it could be compacted into the same int32 as typeReg and hasTypeCheck, saving another 4 bytes for "get" PICs.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

26 bits should be more than enough. The get/callprop ICs have more than one exit block in their slow path, and we used to sync a huge amount of stuff in there. Now that offset can probably be packed in 12 bits.

Just add an assert in Compiler.cpp that after setting it, its value is equal to the distance we expect. Doing that locally, packing in 12-bits, and running jit-tests with --jitflags=m,md, I get no failures.

Comment 3

[Nicholas Nethercote [inactive]]

After reducing typeCheckOffset to 26 bits, I get these get/set/other sizes:

32-bit: 52/64/48 bytes
64-bit: 96/112/96 bytes

Note that "get" and "other" size for 64-bit are the same.  Nice.  Maybe just distinguishing "set" and "non-set" might be enough.  I'll have to measure how common "other" ones are.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: busted patch (against TM 61826:a02c6f4ffe4a)

I had this working, then some changes occurred and I updated and merged and I get this assertion a lot:

  Assertion failure: getPic->hasTypeCheck(), at ../methodjit/PolyIC.cpp:888

Not sure what the problem is.

Furthermore, now that bug 631951 has landed, the memory reduction gained by this patch will be roughly 3--4x smaller, and so may not be worth bothering with.

Comment 5

[Nicholas Nethercote [inactive]]

Doesn't seem worth it now, see comment 4.