Closed Bug 1372013 Opened 3 years ago Closed 3 years ago

ScriptLoader does unnecessary hashtable lookups

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

(Keywords: perf)

Attachments

(1 file)

No description provided.
No longer depends on: 1371094
Comment on attachment 8876503 [details] [diff] [review]
Remove a redundant mFetchingModules.Get before Remove call, and replace Get+Put calls with a LookupRemoveIf call

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

Let's talk about using LookupForAdd below before committing this, but r=me.

::: dom/script/ScriptLoader.cpp
@@ +337,5 @@
> +      if (!aValue) {
> +        aValue = new GenericPromise::Private(__func__);
> +      }
> +      promise = aValue;
> +      return false; // don't remove the entry

Wouldn't LookupForAdd make more sense here?  I guess that doesn't give us direct access to the value after looking it up, which makes things a little awkward, but it seems like LookupForAdd would read better?
Attachment #8876503 - Flags: review?(nfroyd) → review+
> Wouldn't LookupForAdd make more sense here?

I think using LookupForAdd would change the behavior.  Currently, if there
is no entry, Get returns false and the Put never occurs.

But yes, we should probably consider having a Lookup method that returns
an EntrPtr kind of object that you can use to read/write the value, and
optionally remove the entry.  I filed bug 1372317 for this.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dd039a7a8b
Remove a redundant mFetchingModules.Get before Remove call, and replace Get+Put calls with a LookupRemoveIf call.  r=froydnj
https://hg.mozilla.org/mozilla-central/rev/d5dd039a7a8b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.