Closed Bug 1459205 Opened 2 years ago Closed 2 years ago

protocol.js's addDictType call getType for every key twice

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-fission)

Attachments

(1 file)

While looking at bug 1449162's regression, I saw addDictType's write method appear in a couple of profile, like this one: https://perfht.ml/2wbsFLT
It record a run of complicated.netmonitor.* with bug 1449162's patches.
The write method is at line 259. You can see it in this profile under sendReturn call (expand this tree)
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/devtools/shared/protocol.js#243-271
  return types.addType(name, {
    category: "dict",
    specializations: specializations,
    read: (v, ctx) => {
      let ret = {};
      for (let prop in v) {
        if (prop in specializations) {
          ret[prop] = types.getType(specializations[prop]).read(v[prop], ctx);
 // here =============^
        } else {
          ret[prop] = v[prop];
        }
      }
      return ret;
    },

    write: (v, ctx) => {
      let ret = {};
      for (let prop in v) {
        if (prop in specializations) {
          ret[prop] = types.getType(specializations[prop]).write(v[prop], ctx);
 // and here==========^

And while looking at this code it becomes obvious we are calling getType twice for each key. Once in read and another time in write.

DAMP reports a 6% improvements with bug 1454580's DAMP test:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=107aaba9077d54aa9ce6f3fc4e8a14616ef0e29a&newProject=try&newRevision=398988ee3ba005a91b246ae436c09cec2e720c15&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=protocol&framework=1
And 5% with high confidence, when using 1000 repetitions:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=a8b6bac73d3bb0e933f4a613518786eb95bc8973&newProject=try&newRevision=68cc60b40129363f877088c59de52435fde672bb&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1

:jryans, you told me once that it is a bit unfortunate to have to do type check/convertion and ideally we could just forward responses as-is. The more I look into protocol.js performance issues, the more I agree with that!
The main challenge, I think will be to automagically convert actors to form/grip. This is something that I find valuable compared to old actors mechanism and we should keep that.
Comment on attachment 8973226 [details]
Bug 1459205 - Speed up dict type by preventing duplicated calls to `getType` from its write method.

https://reviewboard.mozilla.org/r/241710/#review247550

Great, thanks for finding and fixing this! :) It looks like other usages of `getType` are already caching the result similar to this.

::: devtools/shared/protocol.js:249
(Diff revision 1)
> +  let specTypes = {};
> +  for (let prop in specializations) {
> +    try {
> +      specTypes[prop] = types.getType(specializations[prop]);
> +    } catch (e) {
> +      // Types may not be defined yet. Sometime, we define the type *after* using it, but

Nit: Sometime, we -> Sometimes we
Attachment #8973226 - Flags: review?(jryans) → review+
Assignee: nobody → poirot.alex
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17ea0fb841bf
Speed up dict type by preventing duplicated calls to `getType` from its write method. r=jryans
https://hg.mozilla.org/mozilla-central/rev/17ea0fb841bf
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Blocks: dt-pjs
Product: Firefox → DevTools
Whiteboard: dt-fission
You need to log in before you can comment on or make changes to this bug.