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

Use SmallVector in TypeUpdating::updateParamTypes() #5579

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

kripken
Copy link
Member

@kripken kripken commented Mar 15, 2023

Noticed when profiling for #5561 (comment)

Helps #4165

@kripken kripken requested a review from tlively March 15, 2023 23:51
@@ -229,6 +230,13 @@ class Builder {
ret->finalize(type);
return ret;
}
template<size_t N>
Block* makeBlock(const SmallVector<Expression*, N>& items) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth deduplicating all the different versions of makeBlock into a few templatized versions that can take any iterable for the items?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried again yesterday actually, but I just run into C++ stuff I can't figure out, sadly.

The problem is that we have a version makeBlock(Expression* first) and others that take a list as their single parameter, and it gets confused - it tries to use the list versions for subtypes of Expression, and then errors on not having .size() in them. To fix that, I've tried all manner of std::is_base_of inside of the C++ magic to ignore a template. I also tried more basic SFINAE stuff like ordering them so the erroring one is first. But nothing seems to work for me, and after an hour or so I gave up. Maybe that just isn't possible for some reason?

If you can figure it out though that would be incredible!

Copy link
Member

Choose a reason for hiding this comment

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

@tlively
Copy link
Member

tlively commented Mar 16, 2023

How much does this improve performance, out of curiosity?

@TerrorJack
Copy link
Contributor

I tried the artifacts on this branch to run the same test in the same environment in #5561. The run time is now down to 18s.

@kripken
Copy link
Member Author

kripken commented Mar 16, 2023

@tlively I didn't see a speed difference in my own local tests (perhaps because I can't reproduce the malloc contention issue on my machine). But I verified that on common codebases there are fewer params than this, and it will definitely save some work in those cases, so it looks right in general.

@TerrorJack Interesting, to what is that compared to? I see various numbers in that issue but all are much larger than 18s so I'm not sure which is relevant.

@TerrorJack
Copy link
Contributor

@kripken This is compared to the numbers in the top-level post in #5561.

@kripken
Copy link
Member Author

kripken commented Mar 17, 2023

I'm still confused @TerrorJack , sorry, since the toplevel post contains this:

Result sans mimalloc:

$ time bench ./test.sh
benchmarking ./test.sh
time                 221.9 s    (218.3 s .. 224.6 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 219.3 s    (218.1 s .. 220.3 s)
std dev              1.411 s    (1.155 s .. 1.566 s)
variance introduced by outliers: 19% (moderately inflated)

real    58m35.860s
user    129m50.133s
sys     2395m41.639s

Result with mimalloc:

$ time bench ./test.sh 
benchmarking ./test.sh
time                 14.06 s    (13.38 s .. 14.86 s)
                     1.000 R²   (0.999 R² .. 1.000 R²)
mean                 13.94 s    (13.82 s .. 14.03 s)
std dev              123.8 ms   (76.09 ms .. 151.4 ms)
variance introduced by outliers: 19% (moderately inflated)


real    3m43.584s
user    45m38.783s
sys     0m40.349s

To which of those numbers should the 18 seconds be compared? The 219 seems too high (this PR can't help that much) but the 14 is too low (since you say it decreased, not increased). Apologies, I'm probably not reading something right...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants