crbytes: CommonPrefix #1
Conversation
RaduBerinde
force-pushed
the
common-prefix
branch
from
59519fd
to
264e445
Compare
|
|
||
| // This code is based on compare_amd64.s from Go 1.12.5. | ||
|
|
||
| TEXT ·CommonPrefix(SB),$0-56 |
There was a problem hiding this comment.
If anyone actually wants to look at this, here's the diff against bytes.Compare: https://editor.mergely.com/V6Bvcmbr
RaduBerinde
force-pushed
the
common-prefix
branch
from
264e445
to
98f9cb2
Compare
|
Added arm64 assembly. Diff against bytes.Compare implementation: https://editor.mergely.com/lBD1P7YX |
RaduBerinde
force-pushed
the
common-prefix
branch
from
98f9cb2
to
bc7b6c1
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @jbowens)
a discussion (no related file):
Previously, RaduBerinde wrote…
(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)
If anyone actually wants to look at this, here's the diff against
bytes.Compare: https://editor.mergely.com/V6Bvcmbr
Neat! I didn't look at the assembly implementations in detail, but they looked good on a quick skim. I'll leave the thorough review to @jbowens.
There was a problem hiding this comment.
I started giving this a look, but it's slow going... gotta relearn everything I've paged out
Maybe an
arm64implementation on the way back :)
Added arm64 assembly.
Now your return flight is freed up for an assembly version of https://github.com/cockroachdb/pebble/blob/791b3749feb52978b973d79904fb79632b38046e/sstable/colblk/unsafe_slice.go#L115-L136 😄
Reviewed 5 of 7 files at r1, all commit messages.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @petermattis)
|
Maybe it's easier if we can go through it next week in a video chat? Hehe, good idea about the UnsafeSlice thing, I will take a look. By the way, that code as written assumes little-endian arch correct? |
Yeah, that'd be great
Yeah, it does. We might need to revisit that, as IIRC, there's some talk about officially adding support for one big-endian arch in particular. I'm not sure where else we assume little-endian arch within Pebble |
which part of that existing code is assuming little-endian? |
In https://github.com/cockroachdb/pebble/blob/791b3749feb52978b973d79904fb79632b38046e/sstable/colblk/unsafe_slice.go#L115-L136 we cast directly to |
RaduBerinde
force-pushed
the
common-prefix
branch
from
bc7b6c1
to
506bb99
Compare
|
Updated the AMD64 assembly with a lot more comments and simplified some sections. |
RaduBerinde
force-pushed
the
common-prefix
branch
6 times, most recently
from
7e34d6b
to
e24f2ac
Compare
|
Updated ARM64 assembly with comments as well. |
RaduBerinde
force-pushed
the
common-prefix
branch
from
e24f2ac
to
98d672a
Compare
This commit adds a `crbytes` library with a `CommonPrefix` method. It has a generic implementation that compares 8 bytes at a time and a native `amd64` implementation that is based on the `bytes.Compare` implementation.
RaduBerinde
force-pushed
the
common-prefix
branch
from
98d672a
to
b477973
Compare
|
OK to merge? |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaduBerinde)
This commit adds a
crbyteslibrary with aCommonPrefixmethod. It has a generic implementation that compares 8 bytes at a time and a nativeamd64implementation that is based on thebytes.Compareimplementation.This is a "long flight to Europe" special 😄 Maybe an
arm64implementation on the way back :)This change is