Fix Issue 11017 - Improve performance of toLower etc. #1957
Conversation
Changed to use `appender` instead of repeated `~=`. https://d.puremagic.com/issues/show_bug.cgi?id=11017
|
LGTM. Hopefully, we'll get Nit: add a |
|
@monarchdodra Good point, done. |
|
Fine by me. Does it improve things - any rough numbers ? |
|
Sorry, forgot to mention. For a string of 1 million 'A's it is 5x faster on my machine. Of course, it will vary wildly depending on the string. |
|
A more common case would be "Hello, world" or some such. |
|
3x faster for "Hello, world" |
|
Auto-merge toggled on |
|
@Poita Okay, sold! |
Fix Issue 11017 - Improve performance of toLower etc.
|
This pull request introduced a regression: |
| @@ -6933,28 +6933,29 @@ private S toCase(alias indexFn, uint maxIdx, alias tableFn, S)(S s) @trusted pur | |||
| ushort idx = indexFn(cOuter); | |||
| if(idx == ushort.max) | |||
| continue; | |||
| auto result = s[0 .. i].dup; | |||
| auto result = appender!S(s[0..i]); | |||
There was a problem hiding this comment.
Must have been s[0..i].dup ...
There was a problem hiding this comment.
I would blame the appender API. That line should not compile. Yet it's not a bug, it's by design.
There was a problem hiding this comment.
Why wouldn't it compile? What is wrong with the API?
There was a problem hiding this comment.
appender(s) is safe if s is mutable, but allows clobbering immutable data if s is immutable.
This is bad and wrong because:
- a safe and unsafe operation is behind the same function signature, meaning it's easy for templated code to accidentally lead to immutable data corruption
- it should not hide an unsafe operation behind such an innocent-looking call - it should either force the user to add a cast at the call site, or clearly indicate a dangerous operation in its name (e.g. as
assumeSafeAppendandassumeUniquedo).
There was a problem hiding this comment.
but allows clobbering immutable data
The first thing appender does is look at the capacity of the passed in array, to see if it can append to it in-place or not. If it clobbers anything, immutable or not immutable, then it's a bug.
The call should be safe because of that. The fact that it is clobbering this array at all seems to imply a bug in the implementation. I know appender currently writes "past the un-used capacity without marking it as used", but I've never heard of it "writing to used data".
I'm worried. This is supposed to be text-book example of how to use appender.
There was a problem hiding this comment.
All right, glad to know you found the problem.
There was a problem hiding this comment.
A fix for Appender is NOT underway. Not any time soon anyways. For now, I suggest simply doing:
Appender!S result;
result.reserve(s.length);
put(result, s[0 ..i]);It's one extra line of code, but there is still exactly as much allocation and copying. In fact, it makes 1 less call to s.capacity, so it's even faster. Let's just do that for now.
There was a problem hiding this comment.
A fix for Appender is NOT underway.
@monarchdodra why not? At least task it in bugzilla.
There was a problem hiding this comment.
@monarchdodra why not? At least task it in bugzilla.
I meant not in the comming days. But I guess I can make it my top priority. It's tricky business, so it's not a 1-line patch. And there is more than just 1 bug.
I'll try to have some pulls ready on Wednesday.
There was a problem hiding this comment.
Noice. Would be great to put that in bugzilla and assign it to yourself so we track it better and mention it on the changelog etc. Thanks!
Changed to use
appenderinstead of repeated~=.https://d.puremagic.com/issues/show_bug.cgi?id=11017