Migrated to calloc #2080
Migrated to calloc #2080
Conversation
catap
force-pushed
the
calloc
branch
3 times, most recently
from
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.
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.
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.
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.
@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.
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.
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
callocinstead ofmallocplusmemsetbecause:memsetwas missed and it maight be an issue,memsetis additional call, whencalloccan be very optimized,z.allocand 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
memsetfor freshly allocated memory, that may created unexpected and very difficult to hunt down issues.Also,
allocinside zone never reset memory when a macrosalloc[_]does. This commit also synchronise behaviour of two allocs.