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

std.algorithm.searching: no mapping-specialization for extremum #5001

Merged
merged 10 commits into from
Mar 3, 2017

Conversation

wilzbach
Copy link
Member

In #4265 I showed that extremum (aka minElement and maxElement) can be made a lot faster if we provide a special path for the identity case, because the compiler can then make use of SSE instructions.
In theory the compiler should be smart enough to figure it out himself, but well neither dmd nor ldc are that smart :/

For reference here are the benchmarks from #4265:

dmd -release -O foo.d && ./foo
0 4 secs, 610 ms, 609 μs, and 5 hnsecs
1 3 secs, 769 ms, 89 μs, and 4 hnsecs
ldc -release -O3 foo.d && ./foo
0 1 sec, 345 ms, and 914 μs
1 216 ms, 347 μs, and 1 hnsec

Source for benchmarks: http://sprunge.us/QSWP

This PR does the specialization via overloads whereas #4265 does them via comparison of strings.

@safe pure nothrow unittest
private auto extremum(alias selector = "a < b", Range)(Range r)
if (isInputRange!Range && !isInfinite!Range &&
!is(typeof(unaryFun!selector(ElementType!(Range).init))))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that with string mixins a[0] can be a valid unary and binary function:

 pragma(msg, is(typeof(unaryFun!`a[0]`([0])))); // true
 pragma(msg, is(typeof(binaryFun!`a[0]`([0], [0])))); // true

{
if (selectorFun(r.front, extremeElement))
{
extremeElement = r.front;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears uncovered, why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because most tests were using orderd arrays and the default predicate. It's easy to cover though:

58b1c0b

@wilzbach
Copy link
Member Author

In #4265 I showed that extremum (aka minElement and maxElement) can be made a lot faster if we provide a special path for the identity case, because the compiler can then make use of SSE instructions.

I added this to the changelog.
FYI: it's now possible to preview the changelog, e.g.:

http://dtest.dlang.io/artifact/website-90f9ffa1188c64a9b85c20374f22679f2d3b1a6a-3ccd57ed79e89ad113b428236ca14115/web/changelog/2.074.0.html

(there seems to be some whitespace issues in ddoc, but that's the fault of the changelog generation script)

@wilzbach
Copy link
Member Author

@andralex andralex added the auto-merge-squash label 10 minutes ago
@dlang-bot dlang-bot removed the auto-merge-squash label 4 minutes ago

FYI: This is a new security feature to protect from unapproved changes sneaking in.

@andralex
Copy link
Member

@wilzbach cool! so what did I do wrong?

@wilzbach
Copy link
Member Author

@wilzbach cool! so what did I do wrong?

Nothing - I added the changelog entry.

@andralex
Copy link
Member

Ah, got it. So now I'll re-review and reapply.

@wilzbach
Copy link
Member Author

Ah, got it. So now I'll re-review and reapply.

Yep, that was the idea behind this feature. The auto-tester allows such "silent changes".

Performance improvements for `std.algorithm.searching.{min,max}Element`

$(REF minElement, std, algorithm, searching) and $(REF maxElement, std, algorithm, searching)
are now a lot faster as a special path for the identity case is provided.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"a lot" -> "considerably"

better yet:

"considerably (almost 2x in microbenchmarks)" (or whatever)

i.e. we're trying to avoid colloquialisms

@andralex
Copy link
Member

thx!!

@wilzbach
Copy link
Member Author

[SQUASH] Add shell % prefix to the DMD test

Simple "one char" change in the changelog. Sorry

(there seems to be some whitespace issues in ddoc, but that's the fault of the changelog generation script)

PR: dlang/tools#223

MapType extremeElementMapped = mapFun(extremeElement);

static if (isRandomAccessRange!Range && hasLength!Range)
// direct access via a random access range is faster
static if (isRandomAccessRange!Range)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the check for length here will allow an infinite random access range to use this branch

Copy link
Member Author

@wilzbach wilzbach Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!isInfinite!Range is an excluding constraint above as extremum will be an endless loop on infinite ranges.

alias selectorFun = binaryFun!selector;

// direct access via a random access range is faster
static if (isRandomAccessRange!Range)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a check for length here too

@wilzbach
Copy link
Member Author

wilzbach commented Mar 1, 2017

@JackStouffer I had another look at this PR and discovered that there were still some uncovered bits due to the combinatorial explosion of the alias (aka non-string comparison) approach.
Ideally CodeCov won't complain now :)

@wilzbach wilzbach added this to the 2.074.0 milestone Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants