feat(pointer): better performance through directly managed pointers #8
Conversation
Arqu
force-pushed
the
pointer-performance
branch
from
f8b397e
to
14eaa4c
Compare
pointer.go
Outdated
| // NewPointer creates a Pointer with a pre-allocated block of memory | ||
| // to avoid repeated slice expansions | ||
| func NewPointer() Pointer { | ||
| return make([]string, 0, 32) |
There was a problem hiding this comment.
32 should be made into a constant. Something like defaultPointerCap.
pointer.go
Outdated
| // RawDescendant extends the pointer with 1 or more path tokens | ||
| // The function itself is unsafe but allows for much faster pointer management | ||
| func (p Pointer) RawDescendant(path ...string) (Pointer, error) { | ||
| return append(p, path...), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
This isn't unsafe 😄! If the concern is potentially exceeding the cap, the go runtime will just re-allocate to a new pointer once the cap is exceeded:
https://stackoverflow.com/a/41668362
To that end, what about just calling this Descendant, or NewDescendant, and dropping the error?
There was a problem hiding this comment.
Ah, the "unsafe" part was more aimed at we don't completely re-parse the pointer and deal with the ecape/unescape dance.
Arqu
force-pushed
the
pointer-performance
branch
from
14eaa4c
to
e769648
Compare
pointer.go
Outdated
| if len(str) == 0 { | ||
| str = "/" | ||
| } |
There was a problem hiding this comment.
Are we sure this matches the spec? I'm seeing two new test failures with this change. It may be that the tests are wrong.
=== RUN TestParse
TestParse: pointer_test.go:72: case 3 error mismatch. expected: 'parse ://: missing protocol scheme', got: 'parse "://": missing protocol scheme'
TestParse: pointer_test.go:77: case 5 string output mismatch: expected: '', got: '/'
TestParse: pointer_test.go:77: case 6 string output mismatch: expected: '', got: '/'
But these two used to return the empty string. The failure in test case three is also present on master, and comes from bumping up go versions
Pre-requisite for qri-io/jsonschema#65