Revert imperative Utility.escape from 81d7e2a #102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ashawley
force-pushed
the
utility-escape-unimperative
branch
from
e57fc5f to
9b9f556
Compare
ashawley
force-pushed
the
utility-escape-unimperative
branch
2 times, most recently
from
bfe8d22 to
b78d0be
Compare
ashawley
force-pushed
the
utility-escape-unimperative
branch
from
b78d0be to
140b3da
Compare
ashawley
force-pushed
the
utility-escape-unimperative
branch
from
140b3da to
2983077
Compare
|
needs rebase now. LGTM otherwise and I would suggest we merge in a few days, after the rebase, if there are no concrete objections |
ashawley
force-pushed
the
utility-escape-unimperative
branch
2 times, most recently
from
d7181e2 to
3b917fb
Compare
SethTisue
approved these changes
Feb 15, 2017
ashawley
force-pushed
the
utility-escape-unimperative
branch
from
3b917fb to
0cf7ab0
Compare
ashawley
force-pushed
the
utility-escape-unimperative
branch
from
0cf7ab0 to
070f48a
Compare
Merged
ashawley
force-pushed
the
utility-escape-unimperative
branch
4 times, most recently
from
b6159ae to
5adca87
Compare
ashawley
force-pushed
the
utility-escape-unimperative
branch
3 times, most recently
from
afd3be6 to
a3e3a5f
Compare
ashawley
force-pushed
the
utility-escape-unimperative
branch
4 times, most recently
from
60cfbde to
7456dad
Compare
ashawley
force-pushed
the
utility-escape-unimperative
branch
from
7456dad to
82f11b3
Compare
ashawley
force-pushed
the
utility-escape-unimperative
branch
from
82f11b3 to
98bc791
Compare
|
The final rebase I did last minute was just correcting a typo in the commit log. The code was the same. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current version of
scala.xml.Utility.escapewas contributed in 81d7e2a for scala/bug#3014. The method was changed for an issue concerning control characters. Benchmarking at that time happened to show that Scala 2.7 had either a poor performing implementation forIteratororMapor both. This produced anescapefunction that was "3x-4x" slower than an imperative version using a while-loop and a pattern match.Attached to the Jira issue is an
XmlThing.scalabenchmark program. I've re-run the benchmark program, and there's no evidence that the implementation is any faster than the original code in fc79c58.I modified the
XmlThing.scalaprogram: I limited the test to 1-million iterations, down from 100-million so that the tests would take a second rather than a minute. I then ran the tests in groups of 384, and ran the implementations in different order.I ran it with JMH, as well:
Here's the trial data (JMH is the second sheet):
https://docs.google.com/spreadsheets/d/1xZakvCKpZjaRO1QRMv3WySz4H6nGsNUgSMDm52pqKpM/edit?usp=sharing
Cheers to @vshalts for writing an SBT task that can run a program X times.
https://gist.github.com/vshalts/ed389bfb69b017f4dac7150cf0875529