Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEG.js based parser introduced a performance regression #1646

Closed
ocean90 opened this issue Jul 2, 2017 · 6 comments · Fixed by #1775
Closed

PEG.js based parser introduced a performance regression #1646

ocean90 opened this issue Jul 2, 2017 · 6 comments · Fixed by #1775
Assignees
Labels
[Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Milestone

Comments

@ocean90
Copy link
Member

ocean90 commented Jul 2, 2017

After updating the plugin on a site I noticed a performance decrease by 2-3 seconds.

$ wp profile stage --fields=stage,time,cache_ratio
+------------+---------+-------------+
| stage      | time    | cache_ratio |
+------------+---------+-------------+
| bootstrap  | 1.0175s | 88.3%       |
| main_query | 0.025s  | 90.74%      |
| template   | 4.3431s | 93.54%      |
+------------+---------+-------------+
| total (3)  | 5.3856s | 90.86%      |
+------------+---------+-------------+

vs.

$ wp profile stage --fields=stage,time,cache_ratio --skip-plugins=gutenberg
+------------+---------+-------------+
| stage      | time    | cache_ratio |
+------------+---------+-------------+
| bootstrap  | 1.0935s | 88.09%      |
| main_query | 0.0206s | 90.74%      |
| template   | 0.3426s | 93.54%      |
+------------+---------+-------------+
| total (3)  | 1.4567s | 90.79%      |
+------------+---------+-------------+

Checking further

wp profile stage template --fields=hook,time,cache_ratio
+--------------------------+---------+-------------+
| hook                     | time    | cache_ratio |
+--------------------------+---------+-------------+
| template_redirect:before | 0.001s  |             |
| template_redirect        | 0.0239s | 98.46%      |
| template_include:before  | 0.0008s | 100%        |
| template_include         | 0s      |             |
| wp_head:before           | 0.0028s | 100%        |
| wp_head                  | 0.0904s | 98.31%      |
| loop_start:before        | 0.0287s | 93.58%      |
| loop_start               | 0s      |             |
| loop_end:before          | 3.1046s | 91.02%      |
| loop_end                 | 0s      |             |
| wp_footer:before         | 0.0607s | 92.7%       |
| wp_footer                | 0.0017s | 100%        |
| wp_footer:after          | 0.0002s |             |
+--------------------------+---------+-------------+
| total (13)               | 3.315s  | 96.76%      |
+--------------------------+---------+-------------+

So I removed the filter on the_content and the load was back to normal. I also reverted the do_blocks() function to the previous version and the load was still normal. But once the PEG version is in use the page load increases by ~3 seconds.

Will investigate further to get the exact cause for this.

@swissspidy
Copy link
Member

Sounds like it would be a good idea to bail early when there are absolutely no blocks in a post, e.g. by doing a simple strpos() or regex search.

@ocean90
Copy link
Member Author

ocean90 commented Jul 2, 2017

Looks like it just gets slower the more content you have. You can test with http://www.loremipsum.de/index_e.html. I started with 5 paragraphs and increased the content by 5 more paragraphs for each test.

358 words: 0.062175989151001s
921 words: 0.33092999458313s
1438 words: 0.77467513084412s
1793 words: 1.1339340209961s

@nylen nylen self-assigned this Jul 2, 2017
@dmsnell
Copy link
Member

dmsnell commented Jul 3, 2017

This was foreseen as almost unavoidable. The grammar is both designed to be above-all easy to read and implement (so that more-performant versions can be built from it) and also built for JS consumption. PHP is a pretty bad platform on which to generate the parser due to the ways that the semantics of function calling and closures work. Instead we should prefer a hand-written parser that implements the necessary parts of the grammar for the server side.

@nylen
Copy link
Member

nylen commented Jul 3, 2017

I suspect some of the performance problems here are more related to #1611. I will look into a fix using the existing setup and I am confident that we can achieve a reasonable result here.

PHP is a pretty bad platform on which to generate the parser due to the ways that the semantics of function calling and closures work.

There are no closures in the generated parser - I've removed them from the code because they are not supported on PHP 5.2.

I am also skeptical that function calling semantics are the problem. Can you elaborate on that?

Instead we should prefer a hand-written parser that implements the necessary parts of the grammar for the server side.

It is mandatory that client and server parsing behave the same way. I see two ways to achieve this:

  1. Use the same grammar to do the parsing on the client and the server. Ensure correct behavior first, then address performance concerns.
  2. Put thorough test cases in place to ensure the same parse results on the client and the server. This is in a better place now after Add PHP parser based on PEG.js grammar #1152, but there are still no tests for error cases: invalid nesting of blocks, malformed delimiters, etc. We cannot change to a different server-side parser without first implementing these tests.

@ivankristianto
Copy link

We also having the similar issue of the slow parser cause php maximum execution timeout.
And since the parser use filter the_content, it cause very slow post indexing to Elasticsearch using ElasticPress plugin.

Steps to Reproduce

  1. Activate Gutenberg and add 2 content from the demo content
  2. Save & Publish post
  3. Sync Post to Elasticsearch with ElasticPress

I'm using the Gutenberg v0.3.0 and use Gutenberg Demo content.

@nylen nylen added [Priority] High Used to indicate top priority items that need quick attention [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Type] Bug An existing feature does not function as intended labels Jul 6, 2017
@nylen nylen added this to the Beta 0.4.0 milestone Jul 6, 2017
@nylen
Copy link
Member

nylen commented Jul 6, 2017

I'll implement a fix for this in time for this week's release.

Tug added a commit that referenced this issue Jan 6, 2020
* Use string-array instead of plurals tag in strings.xml

See https://github.com/GlotPress/GlotPress-WP/blob/master/gp-includes/formats/format-android.php

* Update string files

* Update scripts to minimize changes in git diff and keep the same context for android

* Exclude strings from tests

* Fix lint errors

* Bump version to 1.17.1

* Update gutenberg ref following 1.17 merge to gutenberg master

* Update gutenberg ref

* Remove declaration on bridge of unused methods.

* Added bridge code for gutenberg to request a native fullscreen preview for for an image from a URL on iOS.

* Updating bundle and gutenberg reference.

* Updated release notes.

* Update gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Update GB reference.

* Updating bundle after catching up branch and gutenberg submodule.

* Update gutenberg ref

* Update release notes.

* Update UI test

* Pass postType as initial prop on iOS

* Pass postType down to Editor

* Allow Android to set the post type

* Updating gutenberg reference.

* Removed duplicate line from merge.

* updafe test files and iOS version for running locally with xcode 11

* Update gutenberg ref

* Update gutenberg ref

* Update aztec to version that support reversed and start on lists.

* Update Gutenberg to version where list settings are active in native.

* update block list check and capabilities

* replace double click on android

* fix block insertion timeout

* update branch with develop

* Update gutenberg ref

* Update package.json and JS bundle for 1.18.0 release

* Update Gutenberg

* Update Gutenberg

* update gutenberg ref

* Update GB-reference.

* Update release notes.

* revert caps to iPhone Simulator

* Update Gutenberg ref

* Update Release notes

* Update gutenberg/ reference

* Update bundles

* Update GB reference.

* Fix spacing

* Add static method to Media class to create instance using mimeType

* Add flag to track when appending multiple selected media items as blocks

* Introduce mediaSelectionCancelled method in WPAndroidGlueCode

* Set flag to append blocks when multiple = false is not respected

* Only use appendUploadMediaFiles plural version

* Update gutenberg ref

* Update to latest Gutenberg master

* Patch jsdom to implement Element.closest()

* Bring back changes on package.json from 1.7.1

* Add docstring to the function

* Return null as per https://dom.spec.whatwg.org/#dom-element-closest

* Update Aztec version.

* Update GB reference.

* Update gutenberg ref

* Update RELEASE-NOTES.txt

* Update GB reference

* Improve code block style

* Update GB reference.

* Update gutenberg ref

* Update Media mimeType truncation to use enum names

* Add Javadoc for mAppendsMultipleSelectedToSiblingBlocks flag

* Set appends to sibling blocks flag explicitly for all requests

* Add clarifying comment for special block append handling

* Remove singular (unused) appendMedia method

* Update GB reference.

* Update GB reference to master.

* Set appends to sibling blocks flag explicitly for other media pick

* Update GB reference.

* Update GB reference.

* Use lowercase for Media mimeType truncation

* Update Gutenberg

* Update Gutenberg

* Update Gutenberg ref - after fix for caption alignment

* Update bundles

* Point to aztecVersion hash which supports list with start and reverse attribute

* Update bundle and gutenberg ref - fix disappearing image

* Update GB reference.

* Update GB reference.

* Update GB reference.

* Update Gutenberg

* Update Gutenberg ref

* Update GB reference

* Update GB reference.

* Only enable page templates on dev builds

* Update aztec version

* Update aztec version

* Add colors for gallery block

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update GB reference.

* Update Gutenberg ref

* Update Aztec version to 1.14.0

* Make sure if the textColor is changed the default text color is updated.

* Use Slider from react-native-community lib (#1620)

* Use Slider from react-native-community lib

* Update slider version

* Add function to read npm version

* Rengenerate yarn.lock

* Bump node version

* Update readNPMVersion function

* Use Slider from fork

* Update Slider commit

* Use Slider from wordpress-mobile fork

* Add react-native-slider podspec

* Improve babel config

* Correct settings.gradle

* Correct project.pbxproj

* Update ref

* Bump

* Update ref

* Update ref to gutenberg master

* Update ref to gutenberg quick fix

* Update ref to gutenberg master

* Update Aztec editor version.

* Update to iOS 13

* Update Xcode version.

* Use iOS 12 for tests.

* Fix typo

* Update Aztec to fix CI error with xcode 10

* Use iOS 12.2

* Update Appium version.

* Update to appium 1.15.1

* Update to Appium 1.15.1 only in iOS

* Update caps.

* Add Gallery Block (#1498)

* WIP - initial-html.js for gallery testing

* Add parent app media mock for Android

* Update gutenberg reference

* Update gutenberg reference

* Comment out line setting mPendingMediaUploadCallback to null

* Update gutenberg reference

* Generate bundles

* Update gutenberg ref

* Generate bundles

* Generate bundles

* Add some color-studio colors for gallery

* Update gutenberg reference

* Generate bundle

* WIP update ref

* Update gutenberg reference

* Update gutenberg submodule

* Generate bundles

* Add $gray-40 color

* Update gutenberg reference

* Generate bundles

* Update gutenberg reference

* Generate bundles

* Update gutenberg reference

* Generate bundles

* Update gutenberg reference

* Generate bundles

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Restore demo content

* Restore anonymous implementation of GutenbergBridgeJS2Parent

* Update gutenberg reference

* Generate bundles

* Bump up Aztec version on iOS Example app

* Update gb ref

* Generate bundles

* Update gutenberg ref

* Update gutenberg ref (#1646)

* Add release notes for Gallery (#1658)

* Make sure we use iPhone 11 (iOS 13) for build and run tests

* Pooint aztecVersion to develop

* Update Appium for Android tests too.

* Set Appium to 1.15.0

* Update aztec version to 1.3.36

* Update appium to 1.16.0-rc.1

* Update Aztec iOS to 1.14.1

* Update GB reference.

* Activate preformat block on android platform (#1615)

* Updates package.json and JS bundle for 1.19.0 release.

* Update to shorten git commands

Make the git commands a little easier to copy by taking out `$` from the start of the lines.

This also matches with the other commands on the page which do not start with `$`.

* Update Unit Tests headings in Getting Started documentation

The heading "Test" should be "Unit Tests".

The heading "Writing and Running Tests" should be "Writing and Running Unit Tests".

https://github.com/wordpress-mobile/gutenberg-mobile/blob/develop/src/index.test.js

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg ref

* Updating bundles.

* Feat: Navigation Down in InnerBlocks (#1379)

* Add ref to gutenberg repo

* Fix e2e tests

* Update ref to gutenberg master

* Update Gutenberg ref

* Remove empty line between checkboxes

* Updating release notes to show video settings in 1.19

* Updating gutenberg reference to latest 1.19 release change on gutenberg master branch

* Update gutenberg ref

* Sass Transformer: Also look for partials

Add support for [SASS partials](https://sass-lang.com/guide), as is already present [upstream](https://github.com/kristerkari/react-native-sass-transformer/blob/52884dd59582856fa17e2b2e8ca9efc37d412387/index.js#L41-L42).

This should fix the issue introduced by #19159, and discussed there.

* Single quotes

* Also update sass-transformer-inside-gb.js

* Update release notes

* Update gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Update Gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Bundles up to date with merged code from develop

* Add ref to gutenberg repo

* Update ref

* Update ref

* Update ref

* Upgrade the SVG lib to fix #1703

* Upgrade the Video lib to fix #1705

* Upgrade the Slider lib to use node_modules in local npm build

* Update ref

* Upgrade the SVG lib ref

* Upgrade the Video lib ref

* Upgrade the Slider lib ref

* Update ref

* Brings back gb master to normal (#1722)

* Fix/Bring back master to normal (#1724)

* Update Gutenberg ref

* Update Gutenberg ref

Co-authored-by: Tugdual de Kerviler <[email protected]>
Co-authored-by: Matt Chowning <[email protected]>
Co-authored-by: etoledom <[email protected]>
Co-authored-by: Sérgio Estêvão <[email protected]>
Co-authored-by: Cameron Voell <[email protected]>
Co-authored-by: Jorge Bernal <[email protected]>
Co-authored-by: Javon Davis <[email protected]>
Co-authored-by: Maxime Biais <[email protected]>
Co-authored-by: Gerardo Pacheco <[email protected]>
Co-authored-by: Matthew Kevins <[email protected]>
Co-authored-by: Stefanos Togoulidis <[email protected]>
Co-authored-by: Marko Savic <[email protected]>
Co-authored-by: Luke Walczak <[email protected]>
Co-authored-by: Pinar Olguc <[email protected]>
Co-authored-by: Sheri Bigelow <[email protected]>
Co-authored-by: jbinda <[email protected]>
Co-authored-by: Bernie Reiter <[email protected]>
Tug added a commit that referenced this issue Jan 16, 2020
* Use string-array instead of plurals tag in strings.xml

See https://github.com/GlotPress/GlotPress-WP/blob/master/gp-includes/formats/format-android.php

* Update string files

* Update scripts to minimize changes in git diff and keep the same context for android

* Exclude strings from tests

* Fix lint errors

* Bump version to 1.17.1

* Update gutenberg ref following 1.17 merge to gutenberg master

* Update gutenberg ref

* Remove declaration on bridge of unused methods.

* Added bridge code for gutenberg to request a native fullscreen preview for for an image from a URL on iOS.

* Updating bundle and gutenberg reference.

* Updated release notes.

* Update gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Update GB reference.

* Updating bundle after catching up branch and gutenberg submodule.

* Update gutenberg ref

* Update release notes.

* Update UI test

* Pass postType as initial prop on iOS

* Pass postType down to Editor

* Allow Android to set the post type

* Updating gutenberg reference.

* Removed duplicate line from merge.

* updafe test files and iOS version for running locally with xcode 11

* Update gutenberg ref

* Update gutenberg ref

* Update aztec to version that support reversed and start on lists.

* Update Gutenberg to version where list settings are active in native.

* update block list check and capabilities

* replace double click on android

* fix block insertion timeout

* update branch with develop

* Update gutenberg ref

* Update package.json and JS bundle for 1.18.0 release

* Update Gutenberg

* Update Gutenberg

* update gutenberg ref

* Update GB-reference.

* Update release notes.

* revert caps to iPhone Simulator

* Update Gutenberg ref

* Update Release notes

* Update gutenberg/ reference

* Update bundles

* Update GB reference.

* Fix spacing

* Add static method to Media class to create instance using mimeType

* Add flag to track when appending multiple selected media items as blocks

* Introduce mediaSelectionCancelled method in WPAndroidGlueCode

* Set flag to append blocks when multiple = false is not respected

* Only use appendUploadMediaFiles plural version

* Update gutenberg ref

* Update to latest Gutenberg master

* Patch jsdom to implement Element.closest()

* Bring back changes on package.json from 1.7.1

* Add docstring to the function

* Return null as per https://dom.spec.whatwg.org/#dom-element-closest

* Update Aztec version.

* Update GB reference.

* Update gutenberg ref

* Update RELEASE-NOTES.txt

* Update GB reference

* Improve code block style

* Update GB reference.

* Update gutenberg ref

* Update Media mimeType truncation to use enum names

* Add Javadoc for mAppendsMultipleSelectedToSiblingBlocks flag

* Set appends to sibling blocks flag explicitly for all requests

* Add clarifying comment for special block append handling

* Remove singular (unused) appendMedia method

* Update GB reference.

* Update GB reference to master.

* Set appends to sibling blocks flag explicitly for other media pick

* Update GB reference.

* Update GB reference.

* Use lowercase for Media mimeType truncation

* Update Gutenberg

* Update Gutenberg

* Update Gutenberg ref - after fix for caption alignment

* Update bundles

* Point to aztecVersion hash which supports list with start and reverse attribute

* Update bundle and gutenberg ref - fix disappearing image

* Update GB reference.

* Update GB reference.

* Update GB reference.

* Update Gutenberg

* Update Gutenberg ref

* Update GB reference

* Update GB reference.

* Only enable page templates on dev builds

* Update aztec version

* Update aztec version

* Add colors for gallery block

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update GB reference.

* Update Gutenberg ref

* Update Aztec version to 1.14.0

* Make sure if the textColor is changed the default text color is updated.

* Use Slider from react-native-community lib (#1620)

* Use Slider from react-native-community lib

* Update slider version

* Add function to read npm version

* Rengenerate yarn.lock

* Bump node version

* Update readNPMVersion function

* Use Slider from fork

* Update Slider commit

* Use Slider from wordpress-mobile fork

* Add react-native-slider podspec

* Improve babel config

* Correct settings.gradle

* Correct project.pbxproj

* Update ref

* Bump

* Update ref

* Update ref to gutenberg master

* Update ref to gutenberg quick fix

* Update ref to gutenberg master

* Update Aztec editor version.

* Update to iOS 13

* Update Xcode version.

* Use iOS 12 for tests.

* Fix typo

* Update Aztec to fix CI error with xcode 10

* Use iOS 12.2

* Update Appium version.

* Update to appium 1.15.1

* Update to Appium 1.15.1 only in iOS

* Update caps.

* Add Gallery Block (#1498)

* WIP - initial-html.js for gallery testing

* Add parent app media mock for Android

* Update gutenberg reference

* Update gutenberg reference

* Comment out line setting mPendingMediaUploadCallback to null

* Update gutenberg reference

* Generate bundles

* Update gutenberg ref

* Generate bundles

* Generate bundles

* Add some color-studio colors for gallery

* Update gutenberg reference

* Generate bundle

* WIP update ref

* Update gutenberg reference

* Update gutenberg submodule

* Generate bundles

* Add $gray-40 color

* Update gutenberg reference

* Generate bundles

* Update gutenberg reference

* Generate bundles

* Update gutenberg reference

* Generate bundles

* Update gutenberg reference

* Generate bundles

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg reference

* Restore demo content

* Restore anonymous implementation of GutenbergBridgeJS2Parent

* Update gutenberg reference

* Generate bundles

* Bump up Aztec version on iOS Example app

* Update gb ref

* Generate bundles

* Update gutenberg ref

* Update gutenberg ref (#1646)

* Add release notes for Gallery (#1658)

* Make sure we use iPhone 11 (iOS 13) for build and run tests

* Pooint aztecVersion to develop

* Update Appium for Android tests too.

* Set Appium to 1.15.0

* Update aztec version to 1.3.36

* Update appium to 1.16.0-rc.1

* Update Aztec iOS to 1.14.1

* Update GB reference.

* Activate preformat block on android platform (#1615)

* Updates package.json and JS bundle for 1.19.0 release.

* Update to shorten git commands

Make the git commands a little easier to copy by taking out `$` from the start of the lines.

This also matches with the other commands on the page which do not start with `$`.

* Update Unit Tests headings in Getting Started documentation

The heading "Test" should be "Unit Tests".

The heading "Writing and Running Tests" should be "Writing and Running Unit Tests".

https://github.com/wordpress-mobile/gutenberg-mobile/blob/develop/src/index.test.js

* Update gutenberg reference

* Update gutenberg reference

* Update gutenberg ref

* Updating bundles.

* Feat: Navigation Down in InnerBlocks (#1379)

* Add ref to gutenberg repo

* Fix e2e tests

* Update ref to gutenberg master

* Update Gutenberg ref

* Remove empty line between checkboxes

* Updating release notes to show video settings in 1.19

* Updating gutenberg reference to latest 1.19 release change on gutenberg master branch

* Update gutenberg ref

* Sass Transformer: Also look for partials

Add support for [SASS partials](https://sass-lang.com/guide), as is already present [upstream](https://github.com/kristerkari/react-native-sass-transformer/blob/52884dd59582856fa17e2b2e8ca9efc37d412387/index.js#L41-L42).

This should fix the issue introduced by #19159, and discussed there.

* Single quotes

* Also update sass-transformer-inside-gb.js

* Update release notes

* Update gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Update Gutenberg ref

* Update gutenberg ref

* Update gutenberg ref

* Bundles up to date with merged code from develop

* Add ref to gutenberg repo

* Update ref

* Update ref

* Update ref

* Upgrade the SVG lib to fix #1703

* Upgrade the Video lib to fix #1705

* Upgrade the Slider lib to use node_modules in local npm build

* Update ref

* Upgrade the SVG lib ref

* Upgrade the Video lib ref

* Upgrade the Slider lib ref

* Update ref

* Brings back gb master to normal (#1722)

* Fix/Bring back master to normal (#1724)

* Update Gutenberg ref

* Update Gutenberg ref

Co-authored-by: Tugdual de Kerviler <[email protected]>
Co-authored-by: Matt Chowning <[email protected]>
Co-authored-by: etoledom <[email protected]>
Co-authored-by: Sérgio Estêvão <[email protected]>
Co-authored-by: Cameron Voell <[email protected]>
Co-authored-by: Jorge Bernal <[email protected]>
Co-authored-by: Javon Davis <[email protected]>
Co-authored-by: Maxime Biais <[email protected]>
Co-authored-by: Gerardo Pacheco <[email protected]>
Co-authored-by: Matthew Kevins <[email protected]>
Co-authored-by: Stefanos Togoulidis <[email protected]>
Co-authored-by: Marko Savic <[email protected]>
Co-authored-by: Luke Walczak <[email protected]>
Co-authored-by: Pinar Olguc <[email protected]>
Co-authored-by: Sheri Bigelow <[email protected]>
Co-authored-by: jbinda <[email protected]>
Co-authored-by: Bernie Reiter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants