Skip to content

[jemalloc] set correct excess in alloc_excess #45512

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

Closed
wants to merge 1 commit into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Oct 25, 2017

No description provided.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2017
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 25, 2017

@kennytm can you try this?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 25, 2017

@Mark-Simulacrum would it be possible to run a perf of this?

@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Collaborator

bors commented Oct 25, 2017

⌛ Trying commit 15dff15 with merge d413b1c...

bors added a commit that referenced this pull request Oct 25, 2017
[jemalloc] set correct excess in alloc_excess
@bors
Copy link
Collaborator

bors commented Oct 25, 2017

☀️ Test successful - status-travis
State: approved= try=True

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 26, 2017

@Mark-Simulacrum do you have perf on this?

@Mark-Simulacrum
Copy link
Member

Not yet -- I'll run it soon, haven't had a chance yet. @alexcrichton is probably busy with RBR, but if they might be able to get a chance to run it too.

@alexcrichton
Copy link
Member

Is there actually anything in the standard library which uses this API?

Additionally the handling of nallocx is different elsewhere in this file, and maybe that should be unified?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 28, 2017

@alexcrichton

Is there actually anything in the standard library which uses this API?

I don't know but we want to use it from RawVec in the future. Could you run perf?

Additionally the handling of nallocx is different elsewhere in this file, and maybe that should be unified?

That other place is usable_size which returns min and max values. #45514 adds nallocx on realloc_excess, but I wanted to run perf on both changes independently since this touches the allocator.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 30, 2017

In some tests, this results in an increased memory usage (3-5%) accompanied by a run-time reduction of 3-5%. In some other tests, this reduces the memory usage by 3-4%, slightly increasing the run-time (1%). In most cases, this change has no effect.

@bluss what do you think? Maybe we could bench servo with this, but the results go against my intuition: I would expect reduced memory usage accompanied by reduced run-time.

The only explanation I can think of is that initially, vectors have a larger capacity because of the usable size, but this capacity is not enough, so they double to a larger size that they would have done so without the usable size optimization. This produces the higher memory usage. However, they double fewer times than without this optimization, reducing the run-time.

@bluss
Copy link
Member

bluss commented Oct 30, 2017

@gnzlbg I don't know the perf test suite very well, so I can't say if this tells us anything significant.

And, allocator improvements should not be gated on improving rustc IMO, the allocator is more general purpose than that. (Meaning, the whole picture / other applications decide too)

If I understand it correctly, we don't have any hard decisions in this PR? The excess methods are made for exposing the usable size, so it seems like a natural fix.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 1, 2017

I don't really know how to quantify these. I thought that was the whole point of the perf suite, but it seems that it is more about ensuring that the compiler doesn't get slow than about benchmarking general rust applications. Maybe I got its point wrong.

Does servo has "benchmark" tests or something that we could use?

@Mark-Simulacrum
Copy link
Member

Currently perf is intended just for the rust compiler's own performance measurement, yes, though we eventually hope to expand that by adding runtime benchmarks as well. It just hasn't happened yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants