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

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

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 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1459205 - Speed up dict type by preventing duplicated calls to `getType` from its write method.

Review commit: https://reviewboard.mozilla.org/r/241710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241710/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8973226 [details]
Bug 1459205 - Speed up dict type by preventing duplicated calls to `getType` from its write method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241710/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17ea0fb841bf
Speed up dict type by preventing duplicated calls to `getType` from its write method. r=jryans

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/17ea0fb841bf