1108941 - Update the cache rules for template literal call site objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

"Template call sites objects are now canonicalized per realm, based upon their list of raw strings."

js> (a=>a)`a${var1}b${var2}` === (a=>a)`a${var3}b${var4}` // true

Comment 1

[Rajinder Yadav]

Hello I would like to work on this bug.

Comment 2

[Rajinder Yadav]

Just a quick update I won't been able to look into this bug till the start of next week.
I'm still committed to working on this bug.

Comment 3

[Tooru Fujisawa [:arai]]

unassigning for now.

also, adding some basic info about this bug:

Required code change:
  * Implement ES 2017 12.2.9.3 GetTemplateObject [1] steps 1-4 in ProcessCallSiteObjOperation [2] or somewhere else

[1] https://tc39.github.io/ecma262/#sec-gettemplateobject
[2] https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/js/src/vm/Interpreter-inl.h#668

Comment 4

[Rajinder Yadav]

Apology I was not able to work on this Tooru, Tom.

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Rajinder Yadav from comment #4)
> Apology I was not able to work on this Tooru, Tom.

don't worry :)

Comment 6

[Andrea Giammarchi]

This hasn't been fixed in 2 years ... meaning for consistency sake Firefox should always be transpiled since even Babel got this right.

Is there any ETA for this resolution or should I keep transpiling every template string in Firefox?

Thanks for any update.

Comment 7

[Jan de Mooij [:jandem]]

Sounds like this is causing problems for developers so it would be great to have this fixed.

Comment 8

[Andrea Giammarchi]

Indeed, this is making Firefox incapable of recognizing a template already used for a specific node.

It'll be the worst browser performing on hyperHTML
https://github.com/WebReflection/hyperHTML

Please fix this ASAP, or Firefox will need to be targeted as "browser that needs Babel for ES6"

Thank you!

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Implement the per-global template literal registry.

arai for the semantic parts.

jonco for the use of WeakCache and the GC hash map parts, which always worries me.

Comment 10

[Shu-yu Guo [:shu]]

Attachment: Use the template literal registry in Ion.

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8844345 [details] [diff] [review]
Implement the per-global template literal registry.

Review of attachment 8844345 [details] [diff] [review]:
-----------------------------------------------------------------

semantic part looks good, as long as the map becomes strong (both key and value).

::: js/src/jscompartment.cpp
@@ +602,5 @@
>  
> +/* static */ HashNumber
> +TemplateRegistryKey::hash(const TemplateRegistryKey::Lookup& lookup)
> +{
> +    uint32_t length = GetAnyBoxedOrUnboxedInitializedLength(lookup);

shouldn't length be size_t?
(the length won't overflow from 32bit with current implementation tho)

@@ +621,5 @@
> +        return false;
> +
> +    for (uint32_t i = 0; i < length; i++) {
> +        JSAtom* a = &GetAnyBoxedOrUnboxedDenseElement(key.rawStrings, i).toString()->asAtom();
> +        JSAtom* b = &GetAnyBoxedOrUnboxedDenseElement(lookup, i).toString()->asAtom();

since we're using the key object's content, map key should be strong reference.
(that's what you meant by adding note, right?)

@@ +1032,5 @@
>      MOZ_ASSERT(!jitCompartment_);
>      MOZ_ASSERT(!debugEnvs);
>      MOZ_ASSERT(enumerators->next() == enumerators);
>      MOZ_ASSERT(regExps.empty());
> +    MOZ_ASSERT(templateLiteralMap_.empty());

this will require clear call like varNames_,
at least once the map becomes non-weak.

Comment 12

[Jon Coppeard (:jonco)]

Comment on attachment 8844345 [details] [diff] [review]
Implement the per-global template literal registry.

Review of attachment 8844345 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine from a GC perspective, but from the IRC discussion it sounds like it doesn't have the exact behaviour required by the spec.

If you need the values to live forever then I think keys will need to live forever too so we can keep the map entry.  You can do this by removing the WeakCache<> and calling |templateLiteralMap_.trace(trc)| in JSCompartment::trace.

If you only require the values to live as long as the keys you could make the map some kind of weak map but that will be slightly more complicated.

::: js/src/jscompartment.cpp
@@ +648,5 @@
> +            return false;
> +
> +        TemplateRegistryKey key;
> +        key.rawStrings = rawStrings;
> +        if (!templateLiteralMap_.add(p, key, templateObj))

I think you need to use relookupOrAdd here as a GC in e.g. DefineProperty could mutate the hash table.

::: js/src/jscompartment.h
@@ +754,5 @@
> +    // Get a unique template object given a JS array of raw template strings
> +    // and a template object. If a template object is found in template
> +    // registry, that object is returned. Otherwise, the passed-in templateObj
> +    // is added to the registry.
> +    bool getTemplateLiteralObject(JSContext* cx, js::HandleObject rawStrings,

You could make rawStrings a HandleArrayObject to make it clear that it's an array.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8844347 [details] [diff] [review]
Use the template literal registry in Ion.

Review of attachment 8844347 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Jon Coppeard (:jonco) (Away until 20th March) from comment #12)
> Comment on attachment 8844345 [details] [diff] [review]
> Implement the per-global template literal registry.
> 
> Review of attachment 8844345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine from a GC perspective, but from the IRC discussion it sounds
> like it doesn't have the exact behaviour required by the spec.
> 
> If you need the values to live forever then I think keys will need to live
> forever too so we can keep the map entry.  You can do this by removing the
> WeakCache<> and calling |templateLiteralMap_.trace(trc)| in
> JSCompartment::trace.
> 
> If you only require the values to live as long as the keys you could make
> the map some kind of weak map but that will be slightly more complicated.
> 

I guess I'll make the map strong in everything because we specced something that leaks foooooooooreeeeeeeeeeeever.

> ::: js/src/jscompartment.cpp
> @@ +648,5 @@
> > +            return false;
> > +
> > +        TemplateRegistryKey key;
> > +        key.rawStrings = rawStrings;
> > +        if (!templateLiteralMap_.add(p, key, templateObj))
> 
> I think you need to use relookupOrAdd here as a GC in e.g. DefineProperty
> could mutate the hash table.
> 

Ah good catch.

> ::: js/src/jscompartment.h
> @@ +754,5 @@
> > +    // Get a unique template object given a JS array of raw template strings
> > +    // and a template object. If a template object is found in template
> > +    // registry, that object is returned. Otherwise, the passed-in templateObj
> > +    // is added to the registry.
> > +    bool getTemplateLiteralObject(JSContext* cx, js::HandleObject rawStrings,
> 
> You could make rawStrings a HandleArrayObject to make it clear that it's an
> array.

I did that in the beginning, but I'm not sure if it sometimes gets unboxed.

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8844347 [details] [diff] [review]
Use the template literal registry in Ion.

Review of attachment 8844347 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ +2186,5 @@
>  
>        case JSOP_CALLSITEOBJ:
> +      {
> +        JSObject* raw = script()->getObject(GET_UINT32_INDEX(pc) + 1);
> +        JSObject* obj = script()->compartment()->getExistingTemplateLiteralObject(raw);

Sorry I forgot something when reviewing this patch. Usually this will be fine because we have a BaselineScript and the Baseline compiler did the work, but if we are running the arguments analysis we don't have a Baseline script so this could fail.

You could check |info().analysisMode() == Analysis_ArgumentsUsage| and just not canonicalize in that case as the arguments analysis only cares about arguments-usage. Maybe add a test for this too.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe119142fb5
Implement the per-global template literal registry. (r=arai,jonco)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc70760af79
Use the template literal registry in Ion. (r=jandem)
https://hg.mozilla.org/integration/mozilla-inbound/rev/53a2476196b3
Update tests and whitelist failing test262 tests.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad3646f7cc5
Fix #include order to open a CLOSED TREE.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6316fd510838
Followup: don't expect template literal objects to already have been canonicalized during arguments analysis. (r=jandem)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a05d147e438
Followup: fix nonunified builds on a CLOSED TREE.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fe119142fb5
https://hg.mozilla.org/mozilla-central/rev/cdc70760af79
https://hg.mozilla.org/mozilla-central/rev/53a2476196b3
https://hg.mozilla.org/mozilla-central/rev/cad3646f7cc5
https://hg.mozilla.org/mozilla-central/rev/6316fd510838
https://hg.mozilla.org/mozilla-central/rev/1a05d147e438

Comment 20

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/55#JavaScript