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

Optimize String.escaped #1637

Merged
merged 8 commits into from Feb 28, 2018

Conversation

Projects
None yet
6 participants
@nojb
Copy link
Contributor

nojb commented Feb 27, 2018

A small optimization, avoiding some allocation in the fast path.

The patch is due to @alainfrisch, I just packaged it as a PR.

Do we need a Changes entry?

| '\"' | '\\' | '\000'..'\031' | '\127'.. '\255' -> esc := true
| _ -> incr i
done;
!esc

This comment has been minimized.

@xavierleroy

xavierleroy Feb 27, 2018

Contributor

This "while not !finished do ... done" idiom brings bad memory from Pascal. What's wrong with the original recursion? Remember, this is a functional language.

This comment has been minimized.

@alainfrisch

alainfrisch Feb 27, 2018

Contributor

Speedups come from:

  • A while-loop is still faster than tail-rec loops.
  • Not recomputing length for every character.
  • Patterns more friendly to the PM compiler (I was suprised, but the cmm code for the original code is clearly sub-optimal).

I'll post benchmarks later on.

This comment has been minimized.

@xavierleroy

xavierleroy Feb 27, 2018

Contributor

Not recomputing the length is fine. Pascal style is not to be encouraged.

This comment has been minimized.

@alainfrisch

alainfrisch Feb 27, 2018

Contributor

Ok, so I propose:

let needs_escape s =
  let rec needs_escape s n i =
    if i >= n then false else
      match String.unsafe_get s i with
      | '\"' | '\\' | '\n' | '\t' | '\r' | '\b' -> true
      | ' ' .. '~' -> needs_escape s n (i+1)
      | _ -> true
  in
  needs_escape s (String.length s) 0

which gives most of the gain.

This comment has been minimized.

@alainfrisch

alainfrisch Feb 27, 2018

Contributor

Some very quick testing suggests that we go from about 550 Mb/s for the current version to 715 Mb/s with the version above.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 27, 2018

When we discussed adapting the standard library to Bytes/String, we discussed such cases (also String.concat with a single element, etc.), and decided to not perform optimization that would change the "natural" sharing behavior of the function, to not break the (possibly careless) code of -unsafe-string users.

It may be reasonable to open the flood gates now that -unsafe-string is not the default anymore, but I think we should discuss this decision globally, not about just a single function. (The discussion can, of course, happen here.)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 27, 2018

and decided to not perform optimization that would change the "natural" sharing behavior of the function

This is unrelated to the current PR, which only implements differently the way String.escaped checks for its already-existing fast-path.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 27, 2018

Oh, I misread, sorry.

@nojb nojb added the stdlib label Feb 27, 2018

@gasche

gasche approved these changes Feb 27, 2018

Copy link
Member

gasche left a comment

I reviewed the patch and believe that the change is correct. (It's not clear that making it a non-internal function is a readability win, but either ways are fine.)

@yallop

This comment has been minimized.

Copy link
Member

yallop commented Feb 27, 2018

It would be nice if flambda¹ eventually allowed us to avoid hand-optimising loops like this.

For example, perhaps flambda could observe that length s and i don't change within the loop, and that needs_escape doesn't escape, and transform the existing code into the equivalent of the latest version of the code in this PR (b2be212).

Or, even better, perhaps flambda could allow us to avoid writing special purpose loops with unsafe accesses altogether; we could have a high-level function

val exists : (char -> bool) -> string -> bool

and then define needs_escape as

exists (function '\"' | '\\' | '\000'..'\031' | '\127'.. '\255' -> true | _ -> false)

¹ i.e. the optimising middle-end in general

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 27, 2018

It would be nice if flambda¹ eventually allowed us to avoid hand-optimising loops like this.

Indeed, but let's not forget Javascript backends...

aux s (String.length s) 0

let escaped s =
if needs_escape s then

This comment has been minimized.

@alainfrisch

alainfrisch Feb 27, 2018

Contributor

On second thought (sorry), the code could be simplified by directly calling from escaped the recursive function (with the three arguments). It probably improves even a bit the generated bytecode.

let rec aux s n i =
if i >= n then false else
match String.unsafe_get s i with
| '\"' | '\\' | '\000'..'\031' | '\127'.. '\255' -> true

This comment has been minimized.

@alainfrisch

alainfrisch Feb 27, 2018

Contributor

Side note: it is "tempting" to drop the check on the length altogether (also avoiding the need to compute the length), relying on the fact that String.unsafe_get s (String.length s) returns \000. The trouble, in addition to stressing @xavierleroy, is that this doesn't work with js_of_ocaml (and presumably Bucklescript).

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Feb 27, 2018

How much time Lexifi's employees can consume to micro-optimizing a non-time-critical library function?

Fix
@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 27, 2018

We have an actual use case where String.escaped turns out to be called a lot on strings that don't need to be escaped, and where the change discussed here brings an observable speedup (not a factor, but a few percents of the total running time).

@Octachron

This comment has been minimized.

Copy link
Contributor

Octachron commented Feb 27, 2018

Since there is a at least one use case where this change brings "an observable speedup", does this warrant a Changes entry as an user-visible change?

@yallop

This comment has been minimized.

Copy link
Member

yallop commented Feb 27, 2018

On second thought (sorry), the code could be simplified by directly calling from escaped the recursive function (with the three arguments).

In a similar vein, you could avoid the branch in escaped by returning bts (B.escaped (bos s)) and s directly from needs_escaped, rather than returning true and false.

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 28, 2018

In a similar vein, you could avoid the branch in escaped by returning bts (B.escaped (bos s)) and s directly from needs_escaped, rather than returning true and false.

That's right, thanks.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 28, 2018

but then you should really go back to a local function within escape :-)

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 28, 2018

but then you should really go back to a local function within escape :-)

Fair enough, will do :)

@@ -101,17 +101,13 @@ let trim s =
else s

let escaped s =
let rec needs_escape i =
if i >= length s then false else
let rec escape s n i =

This comment has been minimized.

@gasche

gasche Feb 28, 2018

Member

Could this function be called escape_if_needed for clarity? What it is doing is sort of weird.

(I promise to make this my last nitpicking. On this PR.)

This comment has been minimized.

@nojb

nojb Feb 28, 2018

Author Contributor

Done!

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 28, 2018

@nojb Feel free to merge directly after adding the Changes entry.

@alainfrisch alainfrisch self-assigned this Feb 28, 2018

@nojb nojb merged commit 3492756 into ocaml:trunk Feb 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.