Closed Bug 1108941 Opened 9 years ago Closed 7 years ago

Update the cache rules for template literal call site objects

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: 446240525, Assigned: shu)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(2 files)

"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
Hello I would like to work on this bug.
Mentor: arai.unmht
Assignee: nobody → rajindery
Status: NEW → ASSIGNED
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.
Blocks: test262
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
Assignee: rajindery → nobody
Status: ASSIGNED → NEW
Apology I was not able to work on this Tooru, Tom.
(In reply to Rajinder Yadav from comment #4)
> Apology I was not able to work on this Tooru, Tom.

don't worry :)
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.
Sounds like this is causing problems for developers so it would be great to have this fixed.
Flags: needinfo?(shu)
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!
arai for the semantic parts.

jonco for the use of WeakCache and the GC hash map parts, which always worries me.
Attachment #8844345 - Flags: review?(jcoppeard)
Attachment #8844345 - Flags: review?(arai.unmht)
Attachment #8844347 - Flags: review?(jdemooij)
Mentor: arai.unmht
Flags: needinfo?(shu)
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.
Attachment #8844345 - Flags: review?(arai.unmht) → feedback+
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.
Attachment #8844345 - Flags: review?(jcoppeard) → review+
Comment on attachment 8844347 [details] [diff] [review]
Use the template literal registry in Ion.

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

Thanks!
Attachment #8844347 - Flags: review?(jdemooij) → review+
(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 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.
Pushed by shu@rfrn.org:
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.
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad3646f7cc5
Fix #include order to open a CLOSED TREE.
Pushed by shu@rfrn.org:
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.
Assignee: nobody → shu
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: