Skip to content
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

Zone: reset memory #2086

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Zone: reset memory #2086

wants to merge 1 commit into from

Conversation

catap
Copy link
Contributor

@catap catap commented Dec 22, 2020

This commit moved memset from alloc macros to zone.alloc to reset all allocated memory at any case.

@@ -54,6 +54,7 @@ object Zone {
if (rawptr == null) {
throw new OutOfMemoryError(s"Unable to allocate $size bytes")
}
libc.memset(rawptr, 0, size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely correct that having package.scala alloc and z.alloc have the same name but different behaviors when it comes to clearing is a nightmare. I have been
hosed by that more times that I care to admit in public, or even in to myself in the dead and quiet of night.

I believe the discussion of memory handling here is larger than proper for a review comment. I will create an Issue to describe my concerns. In short, I think that
one needs to be able to create either raw or cleared memory.

See line 199 for the case which concerns me, why allocate cleared memory
when one knows that it will be immediately overwritten?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting myself to only the suggested change. From my reading, in most c runtime libraries, especially libc, he combination malloc/memset is not directly equivalent to
calloc and the latter is preferred. At the very least, calloc does a check for
integer overflow (yes, that slows it down by a few cycles). I understand that
calloc for more than 128 KiB can use some memory management techniques to delay the zeroing and otherwise improve performance.

A small improvement, however, a well know one and, IMHO, well worth reaping.
I suspect/believe that you went for a minimal change for existing code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeeTibbert I've spent a lot of time before I realised this small different. And I strongly suggest to apply it because to find this the second time is a nightmare.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are corrrect two methods with the same name and opposite behavior is at best an
H. P. Lovecraft nightmare and probably worse.

I agree with fixing that and propose that the alloctor of cleared memory use calloc instead
of the memset/memcpy of this region. This is for all the reasons that you have described
in your other PR.

This part of the PR is good and, in my opinion should advance. I believe that it would be better
with the suggested calloc change, if only to calloc(1, size).

It is not a direct concern of this PR, but I believe that it needs a prominent release
note. This change increases the likelihood of correct use, but also has an impact on
performance. Devos should be made aware of that.


My concern is that there should be a raw allocator, as well as a cleared allocator.

There are roughly 141 instances of " = alloc" now in the SN project. To advance things, in
another PR and after discussion I could create an allocRaw() and look at each of those
instances and see if it should clear or not. That would be a companion PR and limit
the performance impact of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I mention that your fixing this is a Mercy? Yes, it is, thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeeTibbert technically calloc is the same malloc and memset at least it is true for GNU libc.

Anyway, I see one differences between calloc and malloc + memset and it is a way that allow to optimize calloc to allocate zeros pages from the kernel and allow it to make overcommit for memory for SN application.

I'm thinking about this use case and the more I think the more I agree with idea that it is bad because it open very bad door for SN user that allows to allocate more memory than system has and when applicatio treis to use it, it will be killed by OOM killer.

Basicly this "unsure" is a reason why I've created this trivial PR to fix this issue, and move calloc vs malloc + memset to future release.

@@ -238,7 +238,6 @@ package object unsafe {
val $size = _root_.scala.scalanative.unsafe.sizeof[$T]($tag)
val $ptr = $z.alloc($size)
val $rawptr = $runtime.toRawPtr($ptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I will discuss this in an Issue.

To express my concern in short:

In full generality, one would have both an allocator for cleared memory (this PR edit) and one
for raw memory (before this PR). I suspect that almost all cases, memory allocated on the
stack is going to be immediately overwritten by other data. That is, the clearing is wasted.

A developer knows if they want cleared or raw memory and could/should call the proper
method. The key here is that the method have a name which gives a clue and documentation
which says exactly what it does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To try to describe my thoughts. If there is only one stack allocator (pair?) it should have the existing behavior
to minimize change. It is documented in package.scala as not clearing memory and there is no Zone
equivalent to have opposite behavior.

The question, at least in my mind, if implementing a new stackallocZeroed or stackallocCleared is
worth the added complexity. How one answers that may be a matter of personal programming style, expectation,
and experience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the stackalloc changes could be split out to another so that the z.alloc changes can advance quickly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stackalloc is tricky one. Tehcnically it is {0} and for LLVM that I do have and investigate it adds memset.

Anyway, I haven't got time proof that it is behaviour for all LLVM/clang and not some side effect for macOS.

As soon as I do have some time for SN I'll do this investiagetion and if it is true I'll remove this memset but I don't like blind shot without proof on this case because it is easy to change and very difficult to track.

This commit moved `memset` from `alloc` macros to `zone.alloc` to reset
all allocated memory at any case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants