1847672 - Add name length and hash to JSPropertySpec

Status: ASSIGNED

(Core :: JavaScript Engine, task, P3)

Description

[Tooru Fujisawa [:arai]]

webidl binding generates many JSPropertySpecs during the build, and the length and the hash of them are statically known, and it should be easy to put the length and mozilla::HashStringKnownLength call into the generated code.
with both length and hash known, atomization can skip some steps and checks.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Bug 1847672 - Part 1: Add length and hash to JSPropertySpec. r?jandem!

Existing consumer uses length == UnknownStringLength, which uses the current
behavior.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1847672 - Part 2: Use length and hash in JSPropertySpec and JSFunctionSpec macros. r?jandem!

Some consumer stops using js_*_str, but they're rewritten again in
bug 1847677 patch to use WellKnownAtomId.

Depends on D185726

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Bug 1847672 - Part 3: Add length and hash in auto-generated JSPropertySpec. r?jandem!

Depends on D185727

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Bug 1847672 - Part 4: Add JS::PropertySpecNameToId. r?jandem!

Depends on D185728

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1847672 - Part 5: Use JSPropertySpec::Name in dom::ConstantSpec. r?jandem!

Depends on D185729

Comment 6

[Jan de Mooij [:jandem]]

Do you know how this affects the size of libxul for a shippable build?

Comment 7

[Tooru Fujisawa [:arai]]

With the entire patch stack (bug 1847672, bug 1847677, bug 1847758, bug 1847862, and bug 1847761)
linux 64 shippable libxul.so size increases from 193,702,136 bytes to 194,041,648 bytes (+ 339,512 bytes)

https://treeherder.mozilla.org/jobs?repo=try&revision=1ecc355c5b2783b068e11dcdabf8e60e5d37da05&selectedTaskRun=KWzmS8tjQE2XjTfsKU1rlA.0

I'll check the effect of each change.

Comment 8

[Jan de Mooij [:jandem]]

Maybe also worth checking the installer size or Android APK size because they use compression. The new fields probably can be compressed well.

Comment 9

[Tooru Fujisawa [:arai]]

libxul.so size for each patch:

	libxul.so size	increase from the patch	total increase
before	193,702,136	-	-
bug 1847672 patch	193,815,224	+113,088	+113,088
bug 1847677 patch	193,832,328	+17,104	+130,192
bug 1847758 patch	193,988,752	+156,424	+286,616
bug 1847862 patch	193,916,240	-72,512	+214,104
bug 1847761 patch	194,041,648	+125,408	+339,512

and the android APK size

	geckoview-test_runner.apk size	increase	total increase
before	96,401,228	-	-
bug 1847672 & bug 1847677	96,513,884	+112,656	+112,656
all patches	96,672,370	+158,486	+271,142

Comment 10

[Tooru Fujisawa [:arai]]

there are 3,632 static JSPropertySpec or JSFunctionSpec arrays (206 in SpiderMonkey, and remaining in bindings),
and 20,288 static JSPropertySpec or JSFunctionSpec instances there (1,394 in SpiderMonkey, and remaining in bindings).

this bug's patch adds 4 bytes length and 4 bytes hash, so 8 bytes for each instance (48 -> 56 for JSPropertySpec, 40 -> 48 for JSFunctionSpec),
thus, 162,304 bytes increase is expected in raw size,
and 113,088 bytes increase in libxul.so size above would match the amount...?

then, given that the length is optional for the algorithm (we can use unknown-length whenever it overflows, which won't happen in practice), we could use 2 bytes length and put them into existing padding.
there are 6 bytes padding in JSPropertySpec, and 4 bytes padding in JSFunctionSpec on 64-bit arch.
I'll look into it.

also, perhaps JSFunctionSpec can use 1-byte length if nargs can be reduced from 2 bytes to 1 bytes...?

# JSPropertySpec

0:7     name.{string_, symbol_}
8       attributes_
9       kind_
10      *unused*
11      *unused*
12      *unused*
13      *unused*
14      *unused*
15      *unused*
16-23   u.accessors.getter.{native.op, selfHosted.unused}, u.value.type
24:31   u.accessors.getter.{native.info, selfHosted.funname}, u.value.{string,int32,double_}
32:39   u.accessors.setter.{native.op, selfHosted.unused}
40-47   u.accessors.setter.{native.info, selfHosted.funname}

# JSFunctionSpec

0:7     name.{string_, symbol_}
8:15    call.op
16-23   call.info
24-25   nargs
26-27   flags
28      *unused*
29      *unused*
30      *unused*
31      *unused*
32      selfHostedName

Comment 11

[Tooru Fujisawa [:arai]]

as discusses on chat, I'll focus on WellKnownAtomId (bug 1847677) and the extended atom for embedding (bug 1848278),
which will allow us to skip the atomization for each property/function.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:arai, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 13

[Tooru Fujisawa [:arai]]

I'll revisit this after bug 1847677.