Closed
Bug 1108941
Opened 10 years ago
Closed 8 years ago
Update the cache rules for template literal call site objects
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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)
18.56 KB,
patch
|
jonco
:
review+
arai
:
feedback+
|
Details | Diff | Splinter Review |
3.04 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
"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•8 years ago
|
||
Hello I would like to work on this bug.
Updated•8 years ago
|
Updated•8 years ago
|
Mentor: arai.unmht
Updated•8 years ago
|
Assignee: nobody → rajindery
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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
Comment 4•8 years ago
|
||
Apology I was not able to work on this Tooru, Tom.
Comment 5•8 years ago
|
||
(In reply to Rajinder Yadav from comment #4)
> Apology I was not able to work on this Tooru, Tom.
don't worry :)
Comment 6•8 years ago
|
||
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•8 years ago
|
||
Sounds like this is causing problems for developers so it would be great to have this fixed.
Flags: needinfo?(shu)
Comment 8•8 years ago
|
||
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!
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8844347 -
Flags: review?(jdemooij)
Updated•8 years ago
|
Mentor: arai.unmht
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(shu)
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
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.
Comment 17•8 years ago
|
||
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad3646f7cc5
Fix #include order to open a CLOSED TREE.
Comment 18•8 years ago
|
||
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.
Comment 19•8 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
Assignee: nobody → shu
Comment 20•7 years ago
|
||
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•