20% more performance for popFront(char[]) #4849

UplinkCoder · 2016-10-11T09:54:11Z

As per title.
On average 15% more performance because of smaller code and avoiding bsr

JackStouffer · 2016-10-11T11:34:31Z

Can you post some measurements with code?

UplinkCoder · 2016-10-11T11:43:19Z

okay some measurements

pfold took 122us
pfnew took 116us
pfold took 122us
pfnew took 116us
pfold took 124us

wilzbach · 2016-10-11T11:50:59Z

Can you post some measurements with code?

Maybe we can use this incident to start benchmarking directly in Phobos. A fancy way would be to run benchmarks for every PR to prevent performance regression, but a simpler approach is to at least collect them at a common place (e.g. benchmarks), s.t. (1) in the future enabling automated testing is possible and (2) other people can use these benchmarks for improving upon them (or manually checking regressions).

UplinkCoder · 2016-10-11T12:37:43Z

closed until I found out how I am diverging from the spec.

JackStouffer · 2016-10-12T13:43:33Z

okay some measurements

This doesn't tell me anything. What's the code? What's the data you're using? Are you testing just Unicode strings or are you testing with mixed ascii and Unicode? Etc.

UplinkCoder · 2016-10-12T14:35:28Z

@JackStouffer I am using ASCII mixed with unicode, 80% Ascii, 20% unicode.
The file is called blns.txt
https://github.com/minimaxir/big-list-of-naughty-strings/blob/master/blns.txt
The code of the benchmark is here
http://paste.ofcode.org/3CpieAhkrTYEcSncbPKbrj

UplinkCoder · 2016-10-12T17:33:14Z

I am wondering about the freebsd failure.
It seems related but I have no idea why it occurs.

andralex

@UplinkCoder how are you diverging from the spec?

andralex · 2016-10-12T16:38:06Z

std/range/primitives.d

+        ];
+        /*
+          This implementation of popFront does not special case 0b10xx_xxxx
+          Nither did the one before.


s/Nither/Neither/

andralex · 2016-10-12T16:41:40Z

std/range/primitives.d

+        /*
+          This implementation of popFront does not special case 0b10xx_xxxx
+          Nither did the one before.
+          on illigal strings we may skip too many bytes.


s/on/On/
s/illigal/illegal/

andralex · 2016-10-12T17:44:17Z

std/range/primitives.d

+          This implementation of popFront does not special case 0b10xx_xxxx
+          Nither did the one before.
+          on illigal strings we may skip too many bytes.
+         */


I don't understand this comment. So if the first byte has the shape 0x10xx_xxxx (illegal) we skip one byte which is what we need to do.

... you are right
I had a brain-fart :)

andralex · 2016-10-12T17:47:12Z

Seems the autotester failure is a timeout.

UplinkCoder · 2016-10-12T18:10:46Z

@andralex I do not diverge from the spec anymore.

andralex · 2016-10-12T18:12:28Z

std/range/primitives.d

-                 msbs = 1;
-             }
-             str = str.ptr[min(msbs, str.length) .. str.length];
+            str = str.ptr[min(str.length, charWidthTab[c - 192]) .. str.length];


Use charWidthTab.ptr here, you've already bounds-checked it.

andralex · 2016-10-12T18:14:50Z

OK, I approve this. Regarding the scrutiny: until we have benchmarking for phobos we need to rely on self-reported performance measurements.

UplinkCoder · 2016-10-12T18:22:19Z

Hmm I cannot merge myself ....

wilzbach · 2016-10-12T18:26:08Z

until we have benchmarking for phobos we need to rely on self-reported performance measurements.

Yes, but at least we can put the performance measurements into Phobos (or if wanted another common place), s.t. we can improve upon them & others can run the tests themselves.
I bet this PR will be hard to find for future D enthusiasts and the pastelink might even be gone / expired...

andralex · 2016-10-12T18:29:10Z

@wilzbach well, "until we have benchmarking for phobos".equal("put the performance measurements into Phobos")

That's a project in and of itself, with its own build target, reporting, regression checks etc. etc. etc. I might be able to put someone on it, but it's way out of the scope of this PR.

andralex · 2016-10-12T18:30:02Z

@UplinkCoder I've invited you

UplinkCoder · 2016-10-12T18:40:25Z

@andralex great merged.

wilzbach · 2016-10-12T18:42:19Z

@andralex great merged.

You know - autotester is there for a reason :/

andralex · 2016-10-12T19:22:39Z

@UplinkCoder Please create a user id in the autotester and use its "Toggle automerge" feature to pull PRs. Thanks!

MartinNowak · 2016-10-13T11:33:11Z

Maybe we can use this incident to start benchmarking directly in Phobos. A fancy way would be to run benchmarks for every PR to prevent performance regression, but a simpler approach is to at least collect them at a common place (e.g. benchmarks), s.t. (1) in the future enabling automated testing is possible and (2) other people can use these benchmarks for improving upon them (or manually checking regressions).

We have something like that setup in druntime. A small benchmarks folder and a runner for microbenchmarks. No new std.benchmark or anything like that needed.

MartinNowak · 2016-10-13T11:38:39Z

@UplinkCoder please read and/or enhance https://github.com/dlang/phobos/blob/master/CONTRIBUTING.md and the linked wiki pages.

schveiguy · 2016-10-13T13:59:15Z

@UplinkCoder To clarify, in order to use autotester for merging (I'm not sure if this is documented anywhere, it wasn't when I learned how to do it), click on the link to the auto tester in the list of running tests for your PR, e.g.: https://auto-tester.puremagic.com/pull-history.ghtml?projectid=1&repoid=3&pullid=4849

On the upper left corner is a "Login" link. Click on this, and it will link your github account into the auto-tester (not sure if any setup has to happen first time, I don't remember, it just works for me now). Now, the auto tester will provide at the top summary an "Auto-merge" toggle. Click that, and the auto tester will wait for all tests to pass, and merge on your behalf. Please use this when merging any PRs, it (almost) guarantees the master branch will always build.

please read and/or enhance ...

@MartinNowak I couldn't find anything on auto-tester usage there.

UplinkCoder force-pushed the utf_popFront branch 3 times, most recently from 8975f54 to 6a6ca38 Compare October 11, 2016 10:51

UplinkCoder force-pushed the utf_popFront branch from 6a6ca38 to abe00ae Compare October 11, 2016 11:46

UplinkCoder force-pushed the utf_popFront branch from abe00ae to 2cb33be Compare October 11, 2016 12:16

UplinkCoder changed the title ~~15% more performance for popFront(char[])~~ 5-15% more performance for popFront(char[]) Oct 11, 2016

UplinkCoder force-pushed the utf_popFront branch from 2cb33be to 7e3d336 Compare October 11, 2016 12:26

UplinkCoder closed this Oct 11, 2016

UplinkCoder reopened this Oct 12, 2016

UplinkCoder changed the title ~~5-15% more performance for popFront(char[])~~ 20% more performance for popFront(char[]) Oct 12, 2016

UplinkCoder force-pushed the utf_popFront branch 2 times, most recently from a1934c3 to 6676abd Compare October 12, 2016 16:26

andralex reviewed Oct 12, 2016

View reviewed changes

UplinkCoder force-pushed the utf_popFront branch from 6676abd to 4713fea Compare October 12, 2016 18:06

andralex approved these changes Oct 12, 2016

View reviewed changes

15% more performance for popFront(char[])

8bed6b2

UplinkCoder force-pushed the utf_popFront branch from 4713fea to 8bed6b2 Compare October 12, 2016 18:20

UplinkCoder merged commit 406aa8f into dlang:master Oct 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

20% more performance for popFront(char[]) #4849

20% more performance for popFront(char[]) #4849

UplinkCoder commented Oct 11, 2016 •

edited

Loading

JackStouffer commented Oct 11, 2016 •

edited

Loading

UplinkCoder commented Oct 11, 2016

wilzbach commented Oct 11, 2016

UplinkCoder commented Oct 11, 2016

JackStouffer commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

andralex left a comment

andralex Oct 12, 2016

andralex Oct 12, 2016

andralex Oct 12, 2016

UplinkCoder Oct 12, 2016

andralex commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

andralex Oct 12, 2016

andralex commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

wilzbach commented Oct 12, 2016

andralex commented Oct 12, 2016

andralex commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

wilzbach commented Oct 12, 2016

andralex commented Oct 12, 2016

MartinNowak commented Oct 13, 2016 •

edited

Loading

MartinNowak commented Oct 13, 2016

schveiguy commented Oct 13, 2016

20% more performance for popFront(char[]) #4849

20% more performance for popFront(char[]) #4849

Conversation

UplinkCoder commented Oct 11, 2016 • edited Loading

JackStouffer commented Oct 11, 2016 • edited Loading

UplinkCoder commented Oct 11, 2016

wilzbach commented Oct 11, 2016

UplinkCoder commented Oct 11, 2016

JackStouffer commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

andralex left a comment

Choose a reason for hiding this comment

andralex Oct 12, 2016

Choose a reason for hiding this comment

andralex Oct 12, 2016

Choose a reason for hiding this comment

andralex Oct 12, 2016

Choose a reason for hiding this comment

UplinkCoder Oct 12, 2016

Choose a reason for hiding this comment

andralex commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

andralex Oct 12, 2016

Choose a reason for hiding this comment

andralex commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

wilzbach commented Oct 12, 2016

andralex commented Oct 12, 2016

andralex commented Oct 12, 2016

UplinkCoder commented Oct 12, 2016

wilzbach commented Oct 12, 2016

andralex commented Oct 12, 2016

MartinNowak commented Oct 13, 2016 • edited Loading

MartinNowak commented Oct 13, 2016

schveiguy commented Oct 13, 2016

UplinkCoder commented Oct 11, 2016 •

edited

Loading

JackStouffer commented Oct 11, 2016 •

edited

Loading

MartinNowak commented Oct 13, 2016 •

edited

Loading