Split at every character if using empty string as delimiter #1335
Conversation
| end | ||
|
|
||
| result.push(cur = recover String end) | ||
| let found = match chars |
There was a problem hiding this comment.
Note that this will incur a newly added match cost of "unwrapping the type union" at every iteration of this loop. Has this change been benchmarked for how it affects the base case (non-empty delim)?
I think a more performant solution would be to decide once outside the loop whether the delim is empty or non-empty, then use a different implementation of the loop based on that decision, rather than forcing the same calculation of the same decision at every iteration of the loop.
There was a problem hiding this comment.
I would assume that the cost of the match is negligible compared to the rest of what goes on in this method.
Isn't the match cost in this case simply a branch instruction?
There was a problem hiding this comment.
Simply a branch instruction is not necessarily cheap. We are writing an application where that branch might get called hundreds of thousands of times a second. I haven't tested but under heavy load, jemc's variant sounds like it will perform much better.
There was a problem hiding this comment.
I can fix that.
Would we need an RFC to change the signature of the split method to work on String val instead?
There was a problem hiding this comment.
@malthe - I think it would need an RFC, probably.
malthe
force-pushed
the
string-split-empty
branch
from
d90e7a8
to
a1c2134
Compare
|
Sylvan and I talked during sync, if this has no impact on the normal case, we are good with this but if it causes a performance degradation for the normal use case, we are not in favor of this. |
|
I have submitted an RFC and implementation for a version of this that also changes |
malthe
force-pushed
the
string-split-empty
branch
from
a1c2134
to
fc8bd8a
Compare
|
There's a release underway. Please rebase against master and verify that your CHANGELOG entry appears in the "unreleased" section post rebasing. |
malthe
force-pushed
the
string-split-empty
branch
3 times, most recently
from
0512954
to
98f4ef3
Compare
|
@SeanTAllen – do some of the tests fail because of this pull request or because of some general test instability? |
|
We are having issues with some of the network tests. Its probably unrelated to your change. |
|
This time the one failure appears to be Just Travis' Fault™:
|
|
That's been happening a decent amount today. Probably a result of DDoS on DYN. |
|
CHANGELOG changed due to 0.7.0 release, this probably needs to be rebased against master. |
malthe
force-pushed
the
string-split-empty
branch
from
98f4ef3
to
1421ac5
Compare
malthe
force-pushed
the
string-split-empty
branch
from
1421ac5
to
ff3ca66
Compare
|
This should be ready now. |
|
@malthe I don't see any changes to address the performance concern. this introduces a performance regression in the common case to deal with an edge case. I'm not in favor of this in that case. Given that I'm not sure that I think splitting at every character is the right thing to do with an empty string AND it it going to impact on performance for "the usual cases", I am not in favor of this change at this time. |
|
I'll close this one and promote ponylang/rfcs#46 instead (which currently includes this change). |
No description provided.