Skip to content

Perf: Move multiline xml text encoding out of domToText#4046

Merged
samelhusseini merged 2 commits into
RaspberryPiFoundation:developfrom
eanders-ms:domtotextperf
Jul 14, 2020
Merged

Perf: Move multiline xml text encoding out of domToText#4046
samelhusseini merged 2 commits into
RaspberryPiFoundation:developfrom
eanders-ms:domtotextperf

Conversation

@eanders-ms
Copy link
Copy Markdown
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

A complex regular expression in Blockly.Xml.domToText was running on every save, causing performance problems on large projects. The regexp progressively replaces all \n with 
 in content fields, and was added along with FieldMultilineInput, in #2663.

Proposed Changes

Override toXml and fromXml on FieldMultilineInput and do the conversion there. This simplifies the regexp, and it only runs when there is multiline content in the program, improving performance - especially on large programs.

Performance Comparison

Testing with this MakeCode Arcade game: https://arcade.makecode.com/74548-67333-76127-33508

Time spent in Blockly.Xml.domToText:

Before After
10s 250ms

Resolves

Resolves #4030

Test Coverage

I tested this change in playground.html. Steps I followed:

  1. Add a few paragraph blocks and populate them with multiline content.
  2. Export to Xml. Inspect the Xml to verify content encoding.
  3. Delete all blocks.
  4. Import from Xml. Verify (visually) that the content looks correct.

Also: In the dev console, set breakpoints in the toXml and fromXml methods of FieldMultilineInput to verify they're getting called at the right times.

Tested on:

  • Desktop Chrome

Additional information

Of note: Another document-wide regexp has appeared in domToText, and will likely become a performance issue. You may want to fix it: https://github.com/google/blockly/blob/ba68081d8f0f8e81d76baf370716d5955bae82a1/core/xml.js#L327

I can see how domToText would be a tempting place to fixup formatting issues. It might be worth adding a comment in that method stating: "For performance reasons, don't run regular expressions on the whole document."

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Copy link
Copy Markdown
Contributor

@samelhusseini samelhusseini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CLA. Thank you!

@eanders-ms
Copy link
Copy Markdown
Contributor Author

eanders-ms commented Jul 14, 2020 via email

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@samelhusseini samelhusseini merged commit 2b8208b into RaspberryPiFoundation:develop Jul 14, 2020
@samelhusseini
Copy link
Copy Markdown
Contributor

Thanks!

@eanders-ms eanders-ms deleted the domtotextperf branch July 14, 2020 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants