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

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
7 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

7 years ago
Summary: JM: distinguish Set PICs to save space → JM: distinguish "get", "set" and "other" PICs to save space
(Assignee)

Comment 1

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

Comment 3

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

Updated

7 years ago
Blocks: 615199
(Assignee)

Comment 4

7 years ago
Created attachment 512597 [details] [diff] [review]
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.
(Assignee)

Updated

7 years ago
Blocks: 640457
(Assignee)

Comment 5

6 years ago
Doesn't seem worth it now, see comment 4.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.