internal: Allow building on 32bit (fixes #884)

[imsodin]

I was looking into pebble as a replacement for goleveldb in Syncthing, when our CI came up red for the 32bit cross-builds (#884). This PR fixes those build issues. However while the changes in batchskl look sane to me, I am obviously not familiar with the code, and for manual/rawalloc I have no clue about how the affected methods are used at all, i.e. if the limit for the array size that's < max_uint32 is likely to be problematic or not.
Basically my question now is for people knowing the code-base: Any gotchas with these changes? And more generally, given 32bit has been clearly broken since a while, what's your take on the general stability/usability of pebble on 32bit?

[cockroach-teamcity]

This change is 

[holiman]

We are also looking into pebble as a replacement for leveldb in go-ethereum, and, just like @imsodin ,the 32bit cross build issue is breaking our CI. Getting a resolution on this would be great -- a review or just answer to the questions posted already:

Any gotchas with these changes? And more generally, given 32bit has been clearly broken since a while, what's your take on the general stability/usability of pebble on 32bit?

Thanks!

[imsodin]

They gave the answer in the linked ticket, unfortunately negative:

#1575 (comment)

We only support 64-bit for now, so I'm going to close this out.

If anyone knows a maintained, go-only key-value store that supports 32bit, I'd appreciate pointers (goleveldb had a bunch of new contributions lately, maybe it's going to improve still).

Also I still think it would be better to say so in the readme and remove the 32bit code and instead fail to build - thus it's obvious that it's not usable.

[fjl]

IMHO it would be very good to get this PR in. It's only 50 LOC of change, and will at least allow us to compile pebble on 32bit.

[jbowens]

We're not opposed to 32-bit support as long as it doesn't add any additional maintenance burden.

Reviewed 4 of 6 files at r1, 2 of 2 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @imsodin)

---

internal/batchskl/skl.go line 91 at r3 (raw file):

func init() {
	// Slice size is limited by arch dependendent maximum of int and size of

nit: s/dependendent/dependent/

---

internal/batchskl/skl.go line 92 at r3 (raw file):

func init() {
	// Slice size is limited by arch dependendent maximum of int and size of
	// array element (byte). Cannot use math.MaxInt as that's notassignable to

nit: s/notassignable/not assignable/

---

internal/batchskl/skl.go line 94 at r3 (raw file):

	// array element (byte). Cannot use math.MaxInt as that's notassignable to
	// uint32 on 64bit archs.
	if intBits := unsafe.Sizeof(int(0)) * 8; intBits > 32 {

I think the runtime uses something like this expression, which can be lifted into a const. mind doing that here?

const wordSizeBits = 32 << (^uintptr(0) >> 63) // equivalent to unsafe.Sizeof(uintptr(0))

---

internal/batchskl/skl.go line 97 at r3 (raw file):

		maxNodesSize = math.MaxUint32
	} else {
		maxNodesSize = 1 << (intBits - 1) / 2

can this be manual.MaxArrayLen?

[fjl]

Thanks for reviewing it, @jbowens!

[imsodin]

Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @imsodin and @jbowens)

---

internal/batchskl/skl.go line 94 at r3 (raw file):

Previously, jbowens (Jackson Owens) wrote…

I think the runtime uses something like this expression, which can be lifted into a const. mind doing that here?

const wordSizeBits = 32 << (^uintptr(0) >> 63) // equivalent to unsafe.Sizeof(uintptr(0))

I'have just simplified it to one simple constant based on slice size limit and mem allocation limits (lowest for both coincide here) - added a comment.

---

internal/batchskl/skl.go line 97 at r3 (raw file):

Previously, jbowens (Jackson Owens) wrote…

can this be manual.MaxArrayLen?

We can't use that here, as we are constrained to uint32 while for 64bit manual.MaxArrayLen is much larger than that.

[imsodin]

Addressed or answered review comments.

This needs CI with GOARCH=i386 as well- something else changed since I originally PRed on mainline branch, it's currently failing (didn't have time to look at it yet).

[holiman]

We're still hoping to get this PR in. We cannot really begin making use of Pebble as backend as long as it cannot be compiled on 32bit platforms, and we're looking forward to start using Pebble!

[imsodin]

Then someone needs to do more work on it. I started to rebase to current master and there were new issues. After spending an hour on it without getting close to an end, I stopped. Because again I don't see maintaining this on my own if the project has no interest in 32-bit, so doing more than small fixes is beyond my motivation.
Also if anything wants to continue working on it, I'd propose to first add a CI pipeline running with GOARCH set to something 32bit (I don't think native 32bit is available in GH actions?).

[holiman]

@imsodin Just to clarify: my comment was not directed at you. You have done good job getting it thus far, and fwict have addressed the review-comments.

Seeing as "All checks have passed", I was under the impression that this is on re-review stage now, and that there were not much non-maintainers like you or me can do at the moment, hence the ping.

[imsodin]

My main point, not sure if it that was clear, is that with this PR it still doesn't build on 32bit.

[nicktrav]

Hello all - Apologies for the delayed response. Thank you for your patience here, and thank you @imsodin for your work on this to-date.

I’d like to provide some additional context around 32-bit support. While Pebble will (for the most part) work in standalone setups (i.e. when not part of a CockroachDB deployment), it is developed and tested (almost exclusively) with CockroachDB in mind. Given that CockroachDB currently does not target 32-bit architectures, there has historically been no need for such support in Pebble. Our roadmap is currently driven by Cockroach features. Here is a tracking issue for 32-bit support in CockroachDB in case anyone is interested.

I think the best way forward here would be for someone to pick this up if there is still interest, rebase on latest master and address the build issues mentioned. We can also look at amending the commit to build / test a 32-bit version of the binary. Once everything is passing, we can take another look at the patch.

Thanks again for your patience.

[holiman]

We're working on it,.cc @jwasinger