-
Notifications
You must be signed in to change notification settings - Fork 361
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
Migrated to calloc #2080
base: main
Are you sure you want to change the base?
Migrated to calloc #2080
Conversation
0d47dac
to
9d9e967
Compare
This commit migrated all dynamic memory allocation to `calloc` instead of `malloc` plus `memset` because: - at some places `memset` was missed and it maight be an issue, - `memset` is additional call, when `calloc` can be very optimized, - to respect Java's convention. Java convention and scala inherits it state that all allocated memory has zero value. Scala native allocated memory via malloc inside zone and never calls `memset` for freshly allocated memory, that may created unexpected and very difficult to hunt down issues.
This commit introduced `alloc(n, size)` inside zone that allows to easy allocates dynamic arrays.
@@ -48,13 +48,13 @@ void scalanative_convert_addrinfo(struct addrinfo *in, | |||
socklen_t size; | |||
if (in->ai_addr->sa_family == AF_INET) { | |||
struct scalanative_sockaddr_in *addr = | |||
malloc(sizeof(struct scalanative_sockaddr_in)); | |||
calloc(1, sizeof(struct scalanative_sockaddr_in)); |
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.
Why clear memory that is just going to be overwritten?
Network code is particularly sensitive to speed of execution concerns and regressions.
I believe that all of the calloc
in this file do not increase correctness and decrease
performance and should not happen.
@@ -7,7 +7,7 @@ | |||
int scalanative_convert_sockaddr_in(struct scalanative_sockaddr_in *in, | |||
struct sockaddr_in **out, socklen_t *size) { | |||
struct sockaddr_in *s = | |||
(struct sockaddr_in *)malloc(sizeof(struct sockaddr_in)); | |||
(struct sockaddr_in *)calloc(1, sizeof(struct sockaddr_in)); |
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.
Similar to previously expressed concerns: clearing memory which is not a malloc/immediate_memset
idiom in original code should not happen.
@@ -23,7 +23,6 @@ object SocketHelpers { | |||
var hints = alloc[addrinfo] | |||
var ret = alloc[Ptr[addrinfo]] | |||
|
|||
libc.memset(hints.rawptr, 0, sizeof[addrinfo]) | |||
hints.ai_family = AF_UNSPEC |
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 think that removing the memset is a BUG before/until PR #2086 is merged. Both here and
a second instance below. That is, I think the memset is recovery from the alloc != z.alloc
bug.
There is a slight chance that the memset dates from before (any) alloc cleared memory
and can be safely removed, slightly improving performance.
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.
@LeeTibbert yes, this PR was created before #2086, and I'm still not sure that this one is required as #2086 required.
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.
Thanks to refresh this, let me mark it is a draft to save time by anyone.
import java.lang.Long.toHexString | ||
|
||
class CArrayBoxingTest { | ||
var any: Any = null | ||
|
||
@noinline lazy val nullArr: CArray[Byte, _4] = null | ||
@noinline lazy val arr: CArray[Byte, _4] = !malloc(64.toULong) | ||
@noinline lazy val arr: CArray[Byte, _4] = !calloc(1.toUInt, 64.toUInt) |
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.
What is the advantage of clearing the memory. On a quick read, arr & arr2 are used as addresses and the contents not considered before being set (if ever, I did not trace that).
Have I missed something?
This PR adds a general update for documentation, which includes: * Removed implementation-specific `javalib` classes * Added missing public API `javalib` classes * Updated clang / llvm version requirement (with merged #2080 closes #1824) * Ergys changes according to esentail build requirements resolves #1716 * Updated list of supported scala version for 0.4.0 release * Added information about Commix GC * Added missing bindins in posixlib.rst and clib.rst
This PR adds a general update for documentation, which includes: * Removed implementation-specific `javalib` classes * Added missing public API `javalib` classes * Updated clang / llvm version requirement (with merged scala-native#2080 closes scala-native#1824) * Ergys changes according to esentail build requirements resolves scala-native#1716 * Updated list of supported scala version for 0.4.0 release * Added information about Commix GC * Added missing bindins in posixlib.rst and clib.rst
This PR adds a general update for documentation, which includes: * Removed implementation-specific `javalib` classes * Added missing public API `javalib` classes * Updated clang / llvm version requirement (with merged scala-native#2080 closes scala-native#1824) * Ergys changes according to esentail build requirements resolves scala-native#1716 * Updated list of supported scala version for 0.4.0 release * Added information about Commix GC * Added missing bindins in posixlib.rst and clib.rst
This commit migrated all dynamic memory allocation to
calloc
instead ofmalloc
plusmemset
because:memset
was missed and it maight be an issue,memset
is additional call, whencalloc
can be very optimized,z.alloc
and macrosalloc[_]
.Java convention and scala inherits it state that all allocated memory has zero value.
Scala native allocated memory via malloc inside zone and never calls
memset
for freshly allocated memory, that may created unexpected and very difficult to hunt down issues.Also,
alloc
inside zone never reset memory when a macrosalloc[_]
does. This commit also synchronise behaviour of two allocs.