Closed
Bug 1379900
Opened 7 years ago
Closed 7 years ago
Simplify the XPath query in FormData.collect by doing elements filtering in JS
Categories
(Firefox :: Session Restore, defect)
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: beekill, Assigned: beekill)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 Build ID: 20170616092053
Updated•7 years ago
|
Component: Untriaged → XSLT
Product: Firefox → Core
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Profiling result: + Before the patch: https://perfht.ml/2uLJyYO + After the patch: https://perfht.ml/2ucGwQe
Updated•7 years ago
|
Component: XSLT → Session Restore
Product: Core → Firefox
Updated•7 years ago
|
Assignee: nobody → nnn_bikiu0707
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8885143 [details] Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect. https://reviewboard.mozilla.org/r/156006/#review162528 This is a good start, but I think you can do even more! ::: toolkit/modules/sessionstore/FormData.jsm:134 (Diff revision 1) > get restorableFormNodesXPath() { > // for a comprehensive list of all available <INPUT> types see > // https://dxr.mozilla.org/mozilla-central/search?q=kInputTypeTable&redirect=false > let ignoreInputs = new Map([ > - ["type", ["password", "hidden", "button", "image", "submit", "reset"]], > ["autocomplete", ["off"]] What I'm not sure about is _why_ you needed inputs with 'autocomplete=off' to continue to be filtered using XPath instead of JS. Any particular reason? I'd like to simplify the code here as well by just removing the `ignoreInputs` array altogether. I'd recommend the following structure: ```js const IGNORE_INPUTS = new Map([ ["type", new Set(["password", "hidden"/*, etc*/])], ["autocomplete", new Set(["off"])] ]); function shouldIgnoreNode(node) { for (let [attrName, attrValues] of IGNORE_INPUTS) { if (node.hasAttribute(attrName) && attrValues.has(node.getAttribute(attrName)) { return true; } } return false; } ``` What do you think?
Attachment #8885143 -
Flags: review?(mdeboer) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8885143 [details] Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect. https://reviewboard.mozilla.org/r/156006/#review162746 ::: toolkit/modules/sessionstore/FormData.jsm:134 (Diff revision 1) > get restorableFormNodesXPath() { > // for a comprehensive list of all available <INPUT> types see > // https://dxr.mozilla.org/mozilla-central/search?q=kInputTypeTable&redirect=false > let ignoreInputs = new Map([ > - ["type", ["password", "hidden", "button", "image", "submit", "reset"]], > ["autocomplete", ["off"]] I don't know how to write a function that is simple but still get the job done. So I kept the 'autocomplete=off' to be filtered with using XPath. Thanks for your suggestion :). I changed the Map with array since `for (let ... of ...) {}` is expensive in the loop with many elements (and it does show in the profiler). Array seems to be less expensive.
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8885143 [details] Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect. https://reviewboard.mozilla.org/r/156006/#review162828 r=me with the comment addressed. Did you push this patch to try? ::: toolkit/modules/sessionstore/FormData.jsm:86 (Diff revision 3) > +const IGNORE_ATTRIBUTES = [ > + ["type", new Set(["password", "hidden", "button", "image", "submit", "reset"])], > + ["autocomplete", new Set(["off"])] > +]; > +function shouldIgnoreNode(node) { > + for (let i = 0; i < IGNORE_ATTRIBUTES.length; ++i) { Please use a `for...of` loop here. I'm really surprised to hear that using Maps are way less performant than an Array here, so please file a bug in the 'Core :: Javascript Engine' component so that the JS team can take a look!
Attachment #8885143 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #7) > Please use a `for...of` loop here. I'm really surprised to hear that using > Maps are way less performant than an Array here, so please file a bug in the > 'Core :: Javascript Engine' component so that the JS team can take a look! I want to share you some profiles I did with different ways of looping: + `for ... of ...`: https://perfht.ml/2vh4E1u . The shouldIgnoreMethod takes ~440 ms. + `for ... in ...`: https://perfht.ml/2vgbnZF . The shouldIgnoreMethod takes ~268 ms. + (*): https://perfht.ml/2vgM9tQ . The shouldIgnoreMethod takes ~87 ms. (*) is something similar to current patch: ```js let i = IGNORE_ATTRIBUTES.length; while (i--) { let [attrName, attrValues] = IGNORE_ATTRIBUTES[i]; ... } ``` The reason I said Map is slower than Array is because of the profiles like above, I guess it's because of different ways of looping? So based on the profiles, we should use (*)? However, I don't think it is the case because the time of the UI janks are the same, ~1.1 to ~1.2 seconds. Why is the result profiles like that? I only change the looping method and they show different results. Is this normal in profiler?
Flags: needinfo?(mdeboer)
Comment 9•7 years ago
|
||
(In reply to Bao Quan [:beekill] from comment #8) > Why is the result profiles like that? I only change the looping method and > they show different results. Is this normal in profiler? No, not that I know of! If you can see that _not_ using for...of is noticeably faster, than please keep the patch as it is right now. Mike, when you read command 8, does it ring a bell? Could it be that `for...of` is in fact (much) slower than other iteration methods?
Flags: needinfo?(mdeboer) → needinfo?(mconley)
Comment 10•7 years ago
|
||
Redirecting to jandem who definitely knows way more than I do about the fine details of SpiderMonkey.
Flags: needinfo?(mconley) → needinfo?(jdemooij)
Comment 11•7 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #9) > Could it be that `for...of` is in fact (much) slower than other iteration methods? Yeah for-of is slower than for-in or a C-style loop. This is mostly because ES6 added this heavy-weight iteration protocol: for-of uses iterators that have to allocate/return a {done, value} object each time iterator.next() is called. The JITs can optimize this sometimes but it's not trivial. Please file bugs if this is actually blocking you, ideally with a standalone .htm or .js testcase.
Flags: needinfo?(jdemooij)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
The try result looks good. Dão, can you land this? Thanks!!
Flags: needinfo?(dao+bmo)
Comment 18•7 years ago
|
||
(In reply to Bao Quan [:beekill] from comment #17) > The try result looks good. Dão, can you land this? Thanks!! Please close this issue first: https://reviewboard.mozilla.org/r/156006/#issue-summary I can't do this for you because Reviewboard is stupid.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8885143 [details] Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect. https://reviewboard.mozilla.org/r/156006/#review167612
Assignee | ||
Comment 20•7 years ago
|
||
I just found out I can land the commit myself :).
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Bao Quan [:beekill] from comment #20) > I just found out I can land the commit myself :). Oops! Something is wrong with ReviewBoard that made me think I can land commit :P. Dão, can you land this for me? Thanks :).
Flags: needinfo?(dao+bmo)
Comment 22•7 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a0a41228220b Doing elements filtering in JS to simplify XPath query in FormData::collect. r=mikedeboer
Updated•7 years ago
|
Flags: needinfo?(dao+bmo)
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a0a41228220b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in
before you can comment on or make changes to this bug.
Description
•