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)
DevTools
Framework
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 hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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
Comment 5•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Updated•6 years ago
|
Product: Firefox → DevTools
Assignee | ||
Updated•6 years ago
|
Whiteboard: dt-fission
You need to log in
before you can comment on or make changes to this bug.
Description
•