Improve performance of GC.stats#2164
Improve performance of GC.stats#2164mihails-strasuns-sociomantic wants to merge 1 commit intodlang:masterfrom
GC.stats#2164Conversation
mihails-strasuns-sociomantic
requested review from
DmitryOlshansky,
MartinNowak and
rainers
as code owners
|
Thanks for your pull request, @mihails-strasuns-sociomantic! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + druntime#2164" |
mihails-strasuns-sociomantic
force-pushed
the
faster-stats
branch
from
7755a4a to
7c75f29
Compare
mihails-strasuns-sociomantic
force-pushed
the
faster-stats
branch
from
7c75f29 to
80bb0e7
Compare
mihails-strasuns-sociomantic
requested review from
andralex and
wilzbach
as code owners
mihails-strasuns-sociomantic
force-pushed
the
faster-stats
branch
from
80bb0e7 to
81ca050
Compare
src/gc/stats.d
Outdated
| /** | ||
| * Utility to simplify calculating `core.memory.GC.Stats` | ||
| * | ||
| * Copyright: Copyright Digital Mars 2018. |
There was a problem hiding this comment.
Should be copyright the D Language Foundation
| * | ||
| * Copyright: Copyright Digital Mars 2018. | ||
| * License: $(HTTP www.boost.org/LICENSE_1_0.txt, Boost License 1.0). | ||
| */ |
There was a problem hiding this comment.
Please add:
Source: link to github source code
src/gc/stats.d
Outdated
| import core.memory; | ||
| GC.Stats stats; | ||
|
|
||
| /// Record new memory added to the GC pool from OS |
There was a problem hiding this comment.
The style for Ddoc functions is:
/*********
* Record new memory added to the GC pool from the OS.
* Params:
* bytes = amount of new memory added
*/
Yes, it looks rather redundant, but tools expect this format.
| GC.Stats stats; | ||
|
|
||
| /// Record new memory added to the GC pool from OS | ||
| void added(size_t bytes) |
| auto p = size <= 2048 ? smallAlloc(binTable[size], alloc_size, bits) | ||
| : bigAlloc(size, alloc_size, bits); | ||
|
|
||
| statsTracker.allocated(alloc_size); |
There was a problem hiding this comment.
Since this function is performance critical, being about to turn this statement into a no-op is important.
There was a problem hiding this comment.
What do you mean, pragma(inline)?
There was a problem hiding this comment.
By no-op I mean no code is generated for the statement.
There was a problem hiding this comment.
I am afraid it is not possible - GC.stats is supposed to work for every program at any moment of time, it is not controlled by version or runtime flag.
Though considering how extremely expensive call to alloc is, I am not sure I understand concern about 2 extra integer additions either.
WalterBright
left a comment
WalterBright
left a comment
There was a problem hiding this comment.
Approved with suggested changes.
mihails-strasuns-sociomantic
force-pushed
the
faster-stats
branch
from
81ca050 to
8972aa1
Compare
|
Blocked for now, need to reconsider how this is implemented - current approach is just too fragile. |
|
@mihails-strasuns-sociomantic can we close this while figuring out a better solution? |
|
Yes, I was only keeping it as a reference for internal discussion. For the record, I consider two orthogonal approaches to address this problem:
|
4 participants
Turns
GC.stats()from O(n) to O(1) call, calculating used and free memory on the fly as part of regular GC operation. That makes a very big performance difference forGC.stats()usage in apps that allocate lot of GC memory.Right now it I only confirmed it to produce same stats as previous implementation in synthetic examples. Going to test on some real-world projects in a day or two. For now general feedback from those who know about GC internals would be appreciated.