Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1562133002:
libxml2: linearly optimize XPath expressions. (Closed)
Descriptionlibxml2: linearly optimize XPath expressions.
Some XPath expression nodes keep a back pointer to the last/previous
expression node for optimization purposes. Such pointers will result
in repeated work when walking over the expression
tree/graph, peephole optimizing it. Unacceptable amounts of repeated
work as the expression tree becomes deeper.
Avoid by marking the expression nodes during the optimization pass.
R=scottmg
BUG=573768
Committed: https://crrev.com/0b208a502be9d60929be48056dd4213efd998076
Cr-Commit-Position: refs/heads/master@{#369733}
Committed: https://crrev.com/89ca0415031f1522172e78b96bc4f4da29150cd0
Cr-Commit-Position: refs/heads/master@{#369966}
Patch Set 1 #Patch Set 2 : switch to marking scheme #Patch Set 3 : compilation fix #Patch Set 4 : add README.chromium entry #Patch Set 5 : add missing cacheURI initialization #Patch Set 6 : follow C comment formatting convention #Messages
Total messages: 53 (21 generated)
sigbjornf@opera.com changed reviewers: + cpu@chromium.org, scottmg@chromium.org
please take a look. i'd be happy to try to upstream this afterwards, but I think it makes sense to fix it locally first.
sigbjornf@opera.com changed reviewers: + thakis@chromium.org
+thakis@ for potential advice on how to preferably proceed with fixes to third party libs.
The process for changes to third-party libraries is: 1. File upstream bug with patch 2. Upload patch here with a line in README.chromium pointing at the upstream patch If upstream is quick in merging your patch, you can roll the whole library forward instead in 2 too. Example of the README.chromium approach: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/harfbu... Some third-party projects even keep .patch files around, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/sqlite...
....but why do we care about xslt performance? I thought the plan was to kick this code out eventually. I'm not sure we should be changing it.
On 2016/01/07 19:00:30, Nico wrote: > ....but why do we care about xslt performance? I thought the plan was to kick > this code out eventually. I'm not sure we should be changing it. Thanks for the pointers on how to proceed. If users are locked out of using a Chromium-based browser for their XSLT-based content following the libxml2 upgrade last summer, I think that should be addressed rather than leave it in its current state. If not, we can keep it as a local Opera patch, I suppose.
Since it's fixing a regression I suppose it's fine *shrug* On Thu, Jan 7, 2016 at 2:06 PM, <sigbjornf@opera.com> wrote: > On 2016/01/07 19:00:30, Nico wrote: > >> ....but why do we care about xslt performance? I thought the plan was to >> kick >> this code out eventually. I'm not sure we should be changing it. >> > > Thanks for the pointers on how to proceed. > > If users are locked out of using a Chromium-based browser for their > XSLT-based > content following the libxml2 upgrade last summer, I think that should be > addressed rather than leave it in its current state. If not, we can keep > it as a > local Opera patch, I suppose. > > https://codereview.chromium.org/1562133002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/01/06 21:32:47, sof wrote: > please take a look. > > i'd be happy to try to upstream this afterwards, but I think it makes sense to > fix it locally first. This is fine assuming all Blink layout tests pass, but please link to an upstream bug and update README.chromium with a pointer to that.
Description was changed from ========== XPathOptimizeExpressions(): avoid wasted work for XPATH_OP_VALUE nodes. For optimization purposes, these value nodes keep a back pointer to the last/previous expression node, so when traversing the expression tree in XPathOptimizeExpressions(), avoid repeated traversals by following these back pointers. R= BUG=573768 ========== to ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R= BUG=573768 ==========
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562133002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562133002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562133002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/01/08 15:40:20, sof wrote: > Reported https://bugzilla.gnome.org/show_bug.cgi?id=760325 Please note in README.chromium that this is a local modification, and then lgtm.
On 2016/01/08 18:17:00, scottmg (slow) wrote: > On 2016/01/08 15:40:20, sof wrote: > > Reported https://bugzilla.gnome.org/show_bug.cgi?id=760325 > > Please note in README.chromium that this is a local modification, and then lgtm. Thanks, I will wait on that upstream bug a bit first/next.
Description was changed from ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R= BUG=573768 ========== to ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 ==========
On 2016/01/08 18:22:45, sof wrote: > On 2016/01/08 18:17:00, scottmg (slow) wrote: > > On 2016/01/08 15:40:20, sof wrote: > > > Reported https://bugzilla.gnome.org/show_bug.cgi?id=760325 > > > > Please note in README.chromium that this is a local modification, and then > lgtm. > > Thanks, I will wait on that upstream bug a bit first/next. Added README.chromium entry; no activity on upstream bug, will land what's here.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1562133002/#ps60001 (title: "add README.chromium entry")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562133002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562133002/60001
Message was sent while issue was closed.
Description was changed from ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 ========== to ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 ========== to ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 Committed: https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 Cr-Commit-Position: refs/heads/master@{#369733} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 Cr-Commit-Position: refs/heads/master@{#369733}
Message was sent while issue was closed.
On 2016/01/15 13:47:01, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 > Cr-Commit-Position: refs/heads/master@{#369733} Looks like this has introduced MSAN failures for: fast/events/xsl-onload.xhtml fast/xsl/mozilla-tests.xml virtual/pointerevent/fast/events/xsl-onload.xhtml virtual/trustedeventsdefaultaction/fast/events/xsl-onload.xhtml e.g. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec... STDERR: ==4==WARNING: MemorySanitizer: use-of-uninitialized-value STDERR: #0 0x794719e in ?? third_party/libxml/src/xpath.c:14738:9 STDERR: #1 0x792f780 in xmlXPathOptimizeExpression third_party/libxml/src/xpath.c:14809:5 STDERR: #2 0x792f780 in xmlXPathCtxtCompile third_party/libxml/src/xpath.c:14870:0 STDERR: #3 0x11080df1 in xsltXPathCompileFlags third_party/libxslt/libxslt/xsltutils.c:2324:11 Will revert.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1597603002/ by jsbell@chromium.org. The reason for reverting is: MSAN failure - use of uninitialized value..
Message was sent while issue was closed.
Also, please remove tab characters in xpath.c.
Message was sent while issue was closed.
On 2016/01/15 20:19:26, huangs wrote: > Also, please remove tab characters in xpath.c. Thanks for the revert, not all paths were memset()-clearing. (I don't think we tidy up upstream code wrt style & whitespace, btw.)
Message was sent while issue was closed.
Description was changed from ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 Committed: https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 Cr-Commit-Position: refs/heads/master@{#369733} ========== to ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 Committed: https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 Cr-Commit-Position: refs/heads/master@{#369733} ==========
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562133002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562133002/80001
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562133002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562133002/100001
added missing initialization of cacheURI field; ptal?
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562133002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562133002/100001
Message was sent while issue was closed.
Description was changed from ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 Committed: https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 Cr-Commit-Position: refs/heads/master@{#369733} ========== to ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 Committed: https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 Cr-Commit-Position: refs/heads/master@{#369733} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 Committed: https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 Cr-Commit-Position: refs/heads/master@{#369733} ========== to ========== libxml2: linearly optimize XPath expressions. Some XPath expression nodes keep a back pointer to the last/previous expression node for optimization purposes. Such pointers will result in repeated work when walking over the expression tree/graph, peephole optimizing it. Unacceptable amounts of repeated work as the expression tree becomes deeper. Avoid by marking the expression nodes during the optimization pass. R=scottmg BUG=573768 Committed: https://crrev.com/0b208a502be9d60929be48056dd4213efd998076 Cr-Commit-Position: refs/heads/master@{#369733} Committed: https://crrev.com/89ca0415031f1522172e78b96bc4f4da29150cd0 Cr-Commit-Position: refs/heads/master@{#369966} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/89ca0415031f1522172e78b96bc4f4da29150cd0 Cr-Commit-Position: refs/heads/master@{#369966} |
