compiler,runtime: allow map values >256 bytes

[dgryski]

No description provided.

[dgryski]

Wonder if I should extend the keySize type too..

Edit: And I wonder if the type should be uintptr instead...

[aykevl]

Whatever size we pick, we should really add a new check to make sure the key and value are in range. (Technically not part of the fix but it would be great to have, perhaps as a separate commit).

Edit: And I wonder if the type should be uintptr instead...

Yeah uintptr seems more appropriate to me. That's also the size that is used internally for slices and means that AVR will automatically use a 16-bit size.
On the other hand, limiting key/value sizes to 4GB on 64-bit systems doesn't seem entirely unreasonable to me. So maybe uint16 on <32-bit systems and uint32 on 32+ bits systems? (We don't have a type for that yet). But that probably overcomplicates things.

[deadprogram]

Looks good to me. @aykevl any other feedback before merge?

[dgryski]

Hmm... the first time the CI tests ran there was a OOM from one of the stdlib tests. I think I got another OOM when running the test corpus. I'm not sure if they'll be solved by the precise-GC. But I'm not happy at the moment with merging this until I know what's going on. (That either it is the uintptr change causing the OOM and that the precise GC solves it).

[dgryski]

So indeed as-is this PR causes a corpus OOM-failure for gonum.org/v1/gonum/lapack/gonum and merging the precise GC branch on-top fixes it again 🎉 . That's definitely nice proof that the precise GC works.

[aykevl]

So indeed as-is this PR causes a corpus OOM-failure for gonum.org/v1/gonum/lapack/gonum

Huh, that's kind of unexpected. Unless lapack/gonum creates a massive number of small maps, I don't see how that could easily create an OOM. It's just a few extra bytes. Unless it was already really close to the limit and this pushed it over the limit.

[dgryski]

@aykevl More likely it's the more pointer-sized integers pinning allocations, hence why the precise GC solves the issue.

[aykevl]

That would be rather surprising. Those pointer-sized integers only store relatively small values, far too small to look like a pointer (unless you have absolutely gargantuan types like [0x12345678]byte).

[dgryski]

Shrug. No idea then. I'm fine to hold off merging this if you think we should investigate more.

[dgryski]

Actually the precise GC probably won't affect the map code since the map-internal allocations don't have any associated layout information. Hmm.....

[aykevl]

Actually the precise GC probably won't affect the map code since the map-internal allocations don't have any associated layout information. Hmm.....

It does affect this code though, which is a normal allocation:

tinygo/src/runtime/hashmap.go

Lines 76 to 84 in b5ad81c

	return &hashmap{
	buckets: buckets,
	seed: uintptr(fastrand()),
	keySize: keySize,
	valueSize: valueSize,
	bucketBits: bucketBits,
	keyEqual: keyEqual,
	keyHash: keyHash,
	}

But still, map creation shouldn't be that frequent.

[aykevl]

@dgryski can you try rebasing on the dev branch now that precise GC is in?

[dgryski]

Rebased. Running the test corpus now.

[dgryski]

Boo.

FAIL	gonum.org/v1/gonum/lapack/gonum	53.420s

cmd /Users/dgryski/go/src/github.com/tinygo-org/tinygo/build/tinygo test -v -target= -tags=noasm appengine with err: exit status 1 at dir "/Users/dgryski/go/src/github.com/dgryski/tinygo-test-corpus/_corpus/gonum/gonum/lapack/gonum"

Edit: inconsistent passes if I run it in isolation, even without this particular change (dev at 655075e5:

ok  	gonum.org/v1/gonum/lapack/gonum	96.183s
ok  	gonum.org/v1/gonum/lapack/gonum	99.014s
panic: runtime error: out of memory
FAIL	gonum.org/v1/gonum/lapack/gonum	50.066s
panic: runtime error: out of memory
FAIL	gonum.org/v1/gonum/lapack/gonum	51.998s
ok  	gonum.org/v1/gonum/lapack/gonum	101.897s

[aykevl]

That is a better explanation for why the precise GC had any effect: it probably didn't (but the test was flaky).

[dgryski]

Ok, then this is safe to merge I guess.

[deadprogram]

Thank you @dgryski for working on this, and to @aykevl for review. Now merging.