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.

Sign up for GitHub

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

Jump to bottom

Migrated to calloc #2080

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Migrated to calloc #2080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
992bfa3
Migrated to calloc
catap Dec 17, 2020
f3c7706
Introduce alloc(n, size)
catap Dec 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions javalib/src/main/scala/java/net/PlainSocketImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ private[net] class PlainSocketImpl extends SocketImpl {
override def bind(addr: InetAddress, port: Int): Unit = {
val hints = stackalloc[addrinfo]
val ret = stackalloc[Ptr[addrinfo]]
string.memset(hints.asInstanceOf[Ptr[Byte]], 0, sizeof[addrinfo])
hints.ai_family = socket.AF_UNSPEC
hints.ai_flags = AI_NUMERICHOST
hints.ai_socktype = socket.SOCK_STREAM
Expand Down Expand Up @@ -272,7 +271,6 @@ private[net] class PlainSocketImpl extends SocketImpl {
val inetAddr = address.asInstanceOf[InetSocketAddress]
val hints = stackalloc[addrinfo]
val ret = stackalloc[Ptr[addrinfo]]
string.memset(hints.asInstanceOf[Ptr[Byte]], 0, sizeof[addrinfo])
hints.ai_family = socket.AF_UNSPEC
hints.ai_flags = AI_NUMERICHOST | AI_NUMERICSERV
hints.ai_socktype = socket.SOCK_STREAM
Expand Down
5 changes: 3 additions & 2 deletions javalib/src/main/scala/java/util/zip/Deflater.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,9 @@ object Deflater {
strategy: Int,
noHeader: Boolean): zlib.z_streamp = {
val stream =
stdlib.malloc(sizeof[zlib.z_stream]).asInstanceOf[zlib.z_streamp]
string.memset(stream.asInstanceOf[Ptr[Byte]], 0, sizeof[zlib.z_stream])
stdlib
.calloc(1.toUInt, sizeof[zlib.z_stream])
.asInstanceOf[zlib.z_streamp]
val wbits =
if (noHeader) 15 / -1
else 15
Expand Down
5 changes: 3 additions & 2 deletions javalib/src/main/scala/java/util/zip/Inflater.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,9 @@ private object Inflater {

def createStream(noHeader: Boolean): zlib.z_streamp = {
val stream =
stdlib.malloc(sizeof[zlib.z_stream]).asInstanceOf[zlib.z_streamp]
string.memset(stream.asInstanceOf[Ptr[Byte]], 0, sizeof[zlib.z_stream])
stdlib
.calloc(1.toUInt, sizeof[zlib.z_stream])
.asInstanceOf[zlib.z_streamp]
val wbits: Int =
if (noHeader) 15 / -1
else 15
Expand Down
1 change: 1 addition & 0 deletions nativelib/src/main/scala/scala/scalanative/runtime/libc.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import scalanative.unsafe._
@extern
object libc {
def malloc(size: CSize): RawPtr = extern
def calloc(num: CSize, size: CSize): RawPtr = extern
def free(ptr: RawPtr): Unit = extern
def strlen(str: CString): CSize = extern
def memcpy(dst: RawPtr, src: RawPtr, count: CSize): RawPtr = extern
Expand Down
2 changes: 1 addition & 1 deletion nativelib/src/main/scala/scala/scalanative/unsafe/CVarArgList.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ object CVarArgList {
}

val resultStorage =
z.alloc(sizeof[Long] * storage.size.toULong).asInstanceOf[Ptr[Long]]
z.alloc(storage.size.toUInt, sizeof[Long]).asInstanceOf[Ptr[Long]]
val storageStart = storage.asInstanceOf[LongArray].at(0)
libc.memcpy(toRawPtr(resultStorage),
toRawPtr(storageStart),
Expand Down
13 changes: 10 additions & 3 deletions nativelib/src/main/scala/scala/scalanative/unsafe/Zone.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@ package scala.scalanative
package unsafe

import scala.annotation.implicitNotFound
import scalanative.runtime.{libc, RawPtr, fromRawPtr}
import scala.scalanative.unsigned.UnsignedRichInt
import scalanative.runtime.{RawPtr, fromRawPtr, libc}

/** Zone allocator which manages memory allocations. */
@implicitNotFound("Given method requires an implicit zone.")
trait Zone {

/** Allocates n elements of memory of given size. */
def alloc(n: CSize, size: CSize): Ptr[Byte]

/** Allocates memory of given size. */
def alloc(size: CSize): Ptr[Byte]

Expand Down Expand Up @@ -46,18 +50,21 @@ object Zone {

final override def isClosed: Boolean = closed

final def alloc(size: CSize): Ptr[Byte] = {
final def alloc(n: CSize, size: CSize): Ptr[Byte] = {
if (isClosed) {
throw new IllegalStateException("zone allocator is closed")
}
val rawptr = libc.malloc(size)
val rawptr = libc.calloc(n, size)
if (rawptr == null) {
throw new OutOfMemoryError(s"Unable to allocate $size bytes")
}
node = new Node(rawptr, node)
fromRawPtr[Byte](rawptr)
}

final def alloc(size: CSize): Ptr[Byte] =
alloc(1.toUInt, size)

final override def close(): Unit = {
while (node != null) {
libc.free(node.head)
Expand Down
9 changes: 4 additions & 5 deletions nativelib/src/main/scala/scala/scalanative/unsafe/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,10 @@ package object unsafe {
val runtime = q"_root_.scala.scalanative.runtime"

q"""{
import _root_.scala.scalanative.unsigned.UnsignedRichLong
val $size = _root_.scala.scalanative.unsafe.sizeof[$T]($tag)
val $ptr = $z.alloc($size)
val $ptr = $z.alloc(1.toUInt, $size)
val $rawptr = $runtime.toRawPtr($ptr)
$runtime.libc.memset($rawptr, 0, $size)
$ptr.asInstanceOf[Ptr[$T]]
}"""
}
Expand All @@ -255,10 +255,9 @@ package object unsafe {

q"""{
import _root_.scala.scalanative.unsigned.UnsignedRichLong
val $size = _root_.scala.scalanative.unsafe.sizeof[$T]($tag) * $n.toULong
val $ptr = $z.alloc($size)
val $size = _root_.scala.scalanative.unsafe.sizeof[$T]($tag)
val $ptr = $z.alloc($n.toUInt, $size)
val $rawptr = $runtime.toRawPtr($ptr)
$runtime.libc.memset($rawptr, 0, $size)
$ptr.asInstanceOf[Ptr[$T]]
}"""
}
Expand Down
8 changes: 4 additions & 4 deletions posixlib/src/main/resources/scala-native/netdb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

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.

scalanative_convert_scalanative_sockaddr_in(
(struct sockaddr_in *)in->ai_addr, addr, &size);
out->ai_addr = (struct scalanative_sockaddr *)addr;
} else {
struct scalanative_sockaddr_in6 *addr =
malloc(sizeof(struct scalanative_sockaddr_in6));
calloc(1, sizeof(struct scalanative_sockaddr_in6));
scalanative_convert_scalanative_sockaddr_in6(
(struct sockaddr_in6 *)in->ai_addr, addr, &size);
out->ai_addr = (struct scalanative_sockaddr *)addr;
Expand All @@ -70,7 +70,7 @@ void scalanative_convert_addrinfo(struct addrinfo *in,
out->ai_next = NULL;
} else {
struct scalanative_addrinfo *next_native =
malloc(sizeof(struct scalanative_addrinfo));
calloc(1, sizeof(struct scalanative_addrinfo));
scalanative_convert_addrinfo(in->ai_next, next_native);
out->ai_next = next_native;
}
Expand All @@ -97,7 +97,7 @@ int scalanative_getaddrinfo(char *name, char *service,
return status;
}
struct scalanative_addrinfo *res_native =
malloc(sizeof(struct scalanative_addrinfo));
calloc(1, sizeof(struct scalanative_addrinfo));
scalanative_convert_addrinfo(res_c, res_native);
freeaddrinfo(res_c);
*res = res_native;
Expand Down
6 changes: 3 additions & 3 deletions posixlib/src/main/resources/scala-native/sys/socket_conversions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

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.

*size = sizeof(struct sockaddr_in);
s->sin_family = in->sin_family;
s->sin_port = in->sin_port;
Expand All @@ -20,7 +20,7 @@ int scalanative_convert_sockaddr_in6(struct scalanative_sockaddr_in6 *in,
struct sockaddr_in6 **out,
socklen_t *size) {
struct sockaddr_in6 *s =
(struct sockaddr_in6 *)malloc(sizeof(struct sockaddr_in6));
(struct sockaddr_in6 *)calloc(1, sizeof(struct sockaddr_in6));
*size = sizeof(struct sockaddr_in6);
s->sin6_family = in->sin6_family;
s->sin6_port = in->sin6_port;
Expand All @@ -35,7 +35,7 @@ int scalanative_convert_sockaddr_storage(
struct scalanative_sockaddr_storage *in, struct sockaddr_storage **out,
socklen_t *size) {
struct sockaddr_storage *s =
(struct sockaddr_storage *)malloc(sizeof(struct sockaddr_storage));
(struct sockaddr_storage *)calloc(1, sizeof(struct sockaddr_storage));
*size = sizeof(struct sockaddr_storage);
s->ss_family = in->ss_family;
*out = s;
Expand Down
2 changes: 0 additions & 2 deletions posixlib/src/main/scala/scala/scalanative/runtime/SocketHelpers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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.

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 yes, this PR was created before #2086, and I'm still not sure that this one is required as #2086 required.

Copy link
Contributor Author

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.

hints.ai_protocol = 0
hints.ai_addr = null
Expand Down Expand Up @@ -145,7 +144,6 @@ object SocketHelpers {
var hints = alloc[addrinfo]
var ret = alloc[Ptr[addrinfo]]

libc.memset(hints.rawptr, 0, sizeof[addrinfo])
hints.ai_family = AF_UNSPEC
hints.ai_socktype = SOCK_STREAM
hints.ai_protocol = 0
Expand Down
6 changes: 3 additions & 3 deletions unit-tests/src/test/scala/scala/scalanative/unsafe/CArrayBoxingTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,16 @@ import scalanative.junit.utils.AssertThrows._

import scalanative.unsafe.Nat._
import scalanative.unsigned._
import scalanative.libc.stdlib.malloc
import scalanative.libc.stdlib.calloc
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)
Copy link
Contributor

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?

.asInstanceOf[Ptr[CArray[Byte, _4]]]
@noinline lazy val arr2: CArray[Byte, _4] = !malloc(64.toULong)
@noinline lazy val arr2: CArray[Byte, _4] = !calloc(1.toUInt, 64.toUInt)
.asInstanceOf[Ptr[CArray[Byte, _4]]]

@noinline def f[T](x: T): T = x
Expand Down
6 changes: 3 additions & 3 deletions unit-tests/src/test/scala/scala/scalanative/unsafe/CStructBoxingTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@ import org.junit.Assert._
import scalanative.junit.utils.AssertThrows._

import scalanative.unsigned._
import scalanative.libc.stdlib.malloc
import scalanative.libc.stdlib.calloc
import java.lang.Long.toHexString

class CStructBoxingTest {
var any: Any = null

@noinline lazy val nullStruct: CStruct2[Int, Int] = null
@noinline lazy val struct: CStruct2[Int, Int] = !malloc(64.toULong)
@noinline lazy val struct: CStruct2[Int, Int] = !calloc(1.toUInt, 64.toUInt)
.asInstanceOf[Ptr[CStruct2[Int, Int]]]
@noinline lazy val struct2: CStruct2[Int, Int] = !malloc(64.toULong)
@noinline lazy val struct2: CStruct2[Int, Int] = !calloc(1.toUInt, 64.toUInt)
.asInstanceOf[Ptr[CStruct2[Int, Int]]]

@noinline def f[T](x: T): T = x
Expand Down
6 changes: 3 additions & 3 deletions unit-tests/src/test/scala/scala/scalanative/unsafe/PtrBoxingTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@ import org.junit.Assert._
import scalanative.junit.utils.AssertThrows._

import scalanative.unsigned._
import scalanative.libc.stdlib.malloc
import scalanative.libc.stdlib.calloc
import java.lang.Long.toHexString

class PtrBoxingTest {
import PtrBoxingTest._
var any: Any = null

@noinline lazy val nullPtr: Ptr[Byte] = null
@noinline lazy val ptr: Ptr[Byte] = malloc(64.toULong)
@noinline lazy val ptr2: Ptr[Byte] = malloc(64.toULong)
@noinline lazy val ptr: Ptr[Byte] = calloc(1.toUInt, 64.toUInt)
@noinline lazy val ptr2: Ptr[Byte] = calloc(1.toUInt, 64.toUInt)

@noinline def f[T](x: T): T = x
@noinline def cond(): Boolean = true
Expand Down
8 changes: 4 additions & 4 deletions unit-tests/src/test/scala/scala/scalanative/unsafe/ZoneTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class ZoneTest {

@Test def zoneAllocatorAllocWithApply(): Unit = {
Zone { implicit z =>
val ptr = z.alloc(64.toUInt * sizeof[Int])
val ptr = z.alloc(64.toUInt, sizeof[Int])

assertAccessible(ptr, 64)

Expand All @@ -43,7 +43,7 @@ class ZoneTest {
assertTrue(zone.isOpen)
assertFalse(zone.isClosed)

val ptr = zone.alloc(64.toUInt * sizeof[Int])
val ptr = zone.alloc(64.toUInt, sizeof[Int])

assertAccessible(ptr, 64)

Expand All @@ -59,11 +59,11 @@ class ZoneTest {
@Test def allocThrowsExceptionIfZoneAllocatorIsClosed(): Unit = {
implicit val zone: Zone = Zone.open()

zone.alloc(64.toUInt * sizeof[Int])
zone.alloc(64.toUInt, sizeof[Int])

zone.close()

assertThrows(classOf[IllegalStateException],
zone.alloc(64.toUInt * sizeof[Int]))
zone.alloc(64.toUInt, sizeof[Int]))
}
}