-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Non-atomic memory is cleared twice #14677
Comments
First of all, this is a great find! Memory allocations are a huge performance factor in many Crystal applications. Speeding that up is a great win. I'd like to discuss this independently of the proposed solution #14679 which IMO goes too far in one go. I believe we should take smaller steps:
Adding support for allocating non-zeroed memory as a performance improvement is a separate feature request. |
Copied from #14687 (I mixed up the issues 🤦): Investigating memory allocations in codegen, there are only a few calls:
Of these use cases:
Proposal: Having the four Then #14687 could go and squeeze even more performance. |
The only issue with #14710 is that it changes the public This is a much better move to harmonize it all and you were right all along: we'll want a Maybe do this in three steps?
I believe 1. and 2. would be quickly merged (mere bugfixes), while we can discuss 3. (feature change of the public API). I understand this is getting very boring to deal with. We're splitting our own PRs in a myriad of smaller ones on a daily basis 😫 Note: this is only my personal opinion. Let's wait for the others. |
I recalled another discussion which showed an issue with zeroing out memory in the initialization. It makes it impossible to lazily allocate memory via Ref #11572 (comment) ff. |
What's the point in having It is a public API method. Changing its semantics has performance implications for a lot of user code. We should only do that if there's a really good reason for it. |
Dirty memory can always cause hard-to-fix bugs, so having clean memory be the default could be a gold idea. The main reason is to be able to provide a consistent API together with support for #14687. If we later want to have a non-clearing variant of The way I see it, we currently have three options when wanting to implement #14687:
We definitely want some way of allocating clean pointer-free memory because the compiler expects clean memory for reference types and |
Having a clean default sounds good, of course. But it has a performance cost. AFAIK The issue we're addressing here is that the same memory is cleared twice. We should focus on that and not introduce any other changes in the same step. Also, I don't want to introduce an avoidable change without offering an alternative to retain current performance. |
In LLVM 19 there is now an |
Bug Report
When allocating non-atomic memory using the
pointer_malloc
orallocate
primitives, crystal currently always inserts a LLVM memset intrinsic [1] [2] to clear themalloc
-d memory.However, bdwgc already ensures that all memory allocated using
GC.malloc
is cleared - so this is actually unnecessary.The text was updated successfully, but these errors were encountered: