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

When dumping syncable traits, use a struct for the synced values. #14525

Merged
merged 1 commit into from Dec 19, 2017

Conversation

Projects
None yet
3 participants
@RoosterDragon
Member

RoosterDragon commented Dec 16, 2017

Since most traits have few syncable members, this allows us to avoid allocating an array whose lifetime is only a few ticks long. For traits with more members, we fall back to allocating the array.

Allocating object arrays accounts for 22% of ongoing allocations at game start, this PR reduces it to 2%.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 16, 2017

Member

Do you have a histogram or other stats on the numbers here? I'm wondering whether there is any value with adding additional struct types that can hold more/less elements.

Member

pchote commented Dec 16, 2017

Do you have a histogram or other stats on the numbers here? I'm wondering whether there is any value with adding additional struct types that can hold more/less elements.

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Dec 16, 2017

Member

After running the RA shellmap for ~500 ticks, this was how many DumpSyncTrait calls were made with different counts of syncable members:

Count Occurrences
1 1164340
2 70854
3 55080
4 23683
5 865
6 25356
7 0
8 3470

We could happily just add/remove slots in the struct as we like.

Member

RoosterDragon commented Dec 16, 2017

After running the RA shellmap for ~500 ticks, this was how many DumpSyncTrait calls were made with different counts of syncable members:

Count Occurrences
1 1164340
2 70854
3 55080
4 23683
5 865
6 25356
7 0
8 3470

We could happily just add/remove slots in the struct as we like.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 16, 2017

Member

The majority of instances have only one syncable member, so won't this inflate the memory usage?
Can we introduce a fallback to avoid a wrapper and store the value directly if there's only one without introducing a boxing overhead?

Member

pchote commented Dec 16, 2017

The majority of instances have only one syncable member, so won't this inflate the memory usage?
Can we introduce a fallback to avoid a wrapper and store the value directly if there's only one without introducing a boxing overhead?

@pchote pchote added this to the Next release milestone Dec 16, 2017

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Dec 18, 2017

Member

We could keep a single entry and check if the entry is an object[], assuming that if it's an array we instead deem it to be an array holding multiple entries. But this still requires allocating arrays for greater than one element.

The goal here is to reduce GC allocations, and trading a bit of overhead for that seems fine to me. 1Mb is enough to store 13000 traits & effects per report. The RA shellmap has about 8000.

If I expand the original table a bit, perhaps that makes my choice of 4 seem less arbitrary. Bytes is the size needed by the array which is 4 bytes/element plus an overhead of 8 bytes for the managed object header.

Count Occurrences Bytes Cumulative Allocation Saved %
1 1 164 340 13 972 080 79%
2 70 854 1 133 664 85%
3 55 080 1 101 600 91%
4 23 683 568 392 95%
5 865 24 220 95%
6 25 356 811 392 99%
7 0 0 99%
8 3 470 138 800 100%

I think I came up with a table like this when I was originally messing with this (it was months ago), and 4 seemed like a sweet spot for savings versus overhead.

Member

RoosterDragon commented Dec 18, 2017

We could keep a single entry and check if the entry is an object[], assuming that if it's an array we instead deem it to be an array holding multiple entries. But this still requires allocating arrays for greater than one element.

The goal here is to reduce GC allocations, and trading a bit of overhead for that seems fine to me. 1Mb is enough to store 13000 traits & effects per report. The RA shellmap has about 8000.

If I expand the original table a bit, perhaps that makes my choice of 4 seem less arbitrary. Bytes is the size needed by the array which is 4 bytes/element plus an overhead of 8 bytes for the managed object header.

Count Occurrences Bytes Cumulative Allocation Saved %
1 1 164 340 13 972 080 79%
2 70 854 1 133 664 85%
3 55 080 1 101 600 91%
4 23 683 568 392 95%
5 865 24 220 95%
6 25 356 811 392 99%
7 0 0 99%
8 3 470 138 800 100%

I think I came up with a table like this when I was originally messing with this (it was months ago), and 4 seemed like a sweet spot for savings versus overhead.

When dumping syncable traits, use a struct for the synced values.
Since most traits have few syncable members, this allows us to avoid allocating an array whose lifetime is only a few ticks long. For traits with more members, we fall back to allocating the array.
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 18, 2017

Member

Ok, that's reasonable so lets stick with the current approach.

Member

pchote commented Dec 18, 2017

Ok, that's reasonable so lets stick with the current approach.

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Dec 18, 2017

Member

(Dear people from the future - you can totally change the number of elements if you like - the table was just how I initially decided to use 4! Please don't feel like it magically must be 4 or anything)

Member

RoosterDragon commented Dec 18, 2017

(Dear people from the future - you can totally change the number of elements if you like - the table was just how I initially decided to use 4! Please don't feel like it magically must be 4 or anything)

@pchote

pchote approved these changes Dec 18, 2017

@pchote pchote added the PR: Needs +2 label Dec 18, 2017

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Dec 19, 2017

Member

👍 but I have a question: when do the allocated arrays get cleaned up? We have an object, which is an array, that is a property of a struct. I actually don't know who's going to clean that up and when, since we don't explicitly destroy any of this.

Member

penev92 commented Dec 19, 2017

👍 but I have a question: when do the allocated arrays get cleaned up? We have an object, which is an array, that is a property of a struct. I actually don't know who's going to clean that up and when, since we don't explicitly destroy any of this.

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Dec 19, 2017

Member

The values are attached to the List<TraitReport> Traits or List<EffectReport> Effects on the Report object.

Each time GenerateSyncReport is called, it clears both these lists - which will allow the object[] to be reclaimed by the GC as it will no longer be referenced.

Member

RoosterDragon commented Dec 19, 2017

The values are attached to the List<TraitReport> Traits or List<EffectReport> Effects on the Report object.

Each time GenerateSyncReport is called, it clears both these lists - which will allow the object[] to be reclaimed by the GC as it will no longer be referenced.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Dec 19, 2017

Member

OK, good enough for me. I trust you know your stuff ;)

Member

penev92 commented Dec 19, 2017

OK, good enough for me. I trust you know your stuff ;)

@penev92 penev92 merged commit 987d0b7 into OpenRA:bleed Dec 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92
Member

penev92 commented Dec 19, 2017

@RoosterDragon RoosterDragon deleted the RoosterDragon:sync-values-struct branch Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment