Closed Bug 1459205 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

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
Status: NEW → RESOLVED
Closed: 7 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.

Attachment

General

Created:
Updated:
Size: