-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
Optimize / refactor CowData
, combining resize and fork to avoid unnecessary reallocations.
#100619
base: master
Are you sure you want to change the base?
Conversation
749d0a5
to
3a4c1f9
Compare
Hrm. I'm kinda stumped as to what's leaking. I think it's foolproof, in any case foolproof if the |
c45c762
to
d69cb3b
Compare
d69cb3b
to
84b402c
Compare
Oh my god... This leak is costing me and arm and a leg to debug. Here is a failing log for reference: https://github.com/godotengine/godot/actions/runs/12435300159/job/34720869143 I have finally tracked it down to the GDScript tests only. Normal operation does not appear to cause leaks. At this point, I am reasonably sure it's not my implementation that's causing it, but some kind of circular reference. Though it's hard to be completely certain. Running the unit tests locally without |
I have an update. Turns out one of the 'preconditions' I was imagining to be true was false all along - This is true on master as well, as this fails unit tests: master...Ivorforce:godot:cowdata-refcount-0 After meticulously re-creating my PR step by step, I'm pretty sure that's what's causing the issues. I don't know how it's possible that |
fe0c304
to
69c3f06
Compare
I think I finally fixed it. So, Instead, I managed to handle it more graciously through #100694. With the change, |
69c3f06
to
2617af3
Compare
@Ivorforce after the fix in cowdata, has this been updated? |
You mean #100694? Yep, I've integrated the changes and tested it's still valid! |
_ptr = _data_ptr; | ||
const Error error = _alloc(alloc_size); | ||
if (error) { | ||
return error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if we cannot alloc, this instance becomes null, and the old data gets destructed. Was this the old behaviour too ? (just making sure we do not have a subtle behaviour difference, because this branch should not get called much)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good observation! I remember thinking a lot about this problem too.
The old behavior was quite curious:
It would first try to allocate an array with the same size as the old one through _copy_on_write
.
If that failed, it would ignore the error and continue to reallocate the old array if needed, potentially invalidating the old pointer (which would likely lead to a crash later).
If it succeeded, it would try to reallocate the fresh pointer to a new size. If that failed, it would keep the old freshly created backing array of the wrong size, and return to the caller (who would most likely ignore the failed resize and subsequently SEGFAULT).
What I'm doing instead is to let the old data go and resize to size 0. What follows is one of two things:
Either the caller realizes the resize failed, and will either crash or report back to the user (who will likely crash soon too, because who checks the output of resize
?). I think it's unlikely that a resize
rather than a new allocation will be out of memory, but still handleable on failure.
Or the caller doesn't realize the resize failed, and will immediately SEGFAULT because of out of bounds access.
It's pretty impossible to restore the old behavior, but I don't think we want to.
There is definitely an argument to be made though to try to recover / keep the old data though. I thought it might be better to encourage early failure by letting the old data go and not attempting to recover it. But I'm not married to the idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't want the old behaviour, no. To be honest I like how you handle it right now, it make it clear that there was an error. It would maybe be better to add a comment to make clear it's on purpose and not some oversight.
core/templates/cowdata.h
Outdated
memnew_placement(&_ptr[i], T); | ||
} | ||
} else if (p_ensure_zero) { | ||
memset((void *)(_ptr + current_size), 0, (p_size - current_size) * sizeof(T)); | ||
memset((void *)&_ptr[prev_size], 0, (p_size - prev_size) * sizeof(T)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would have stick with (void *)(_ptr + prev_size)
but that's style, so both will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about this, IIRC i adjusted this to be on-par with other lines using the []
syntax.
|
||
// If we realloc, we're guaranteed to be the only reference. | ||
new (_refc_ptr) SafeNumeric<USize>(1); | ||
_ptr = _data_ptr; | ||
// So the reference was 1 and was copied to be 1 again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add an assert for that for debug builds ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there's nothing like DEBUG_ASSERT
, so I think DEV_ASSERT
will be good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably yes
2617af3
to
46156c2
Compare
… unnecessary reallocations.
46156c2
to
39997ae
Compare
This PR optimizes
CowData
resize
operations. This affects all non-trivial resizes forString
andVector
(e.g.Packed
arrays), including concatenation.This PR includes #100694, because without it, leaks are occurring due to cyclic references that the previous implementation luckily avoided through unnecessary forking.This PR also supersedes #100672.
Reasoning
The current implementation of
resize
first forks if there are multiple owners, and then resizes the freshly forked array.In the case that the needed array is larger than the previous array, time is wasted because a new location may have to be found for the larger buffer on the subsequent
realloc
, and the data has to be copied over again.In the case that the needed array is smaller than the previous array, time is wasted looking for a larger buffer than necessary, and constructing (and subsequently destructing) more elements than necessary.
Benchmark
I've benchmarked plain String concatenation through
+
, which can be 1.6x as fast as before. The PR is likely to affect and optimize many more functions involvingString
orVector
.This yields:
1349ms
onmaster
(8483d79 / ba2c5c1)825ms
on this PR (77d8900235a8b0fe7e3b597d4f664533f60ee82e)