Conversation
|
#18 is linked to this PR, as if it fixes the issue. But if I understand correctly, the issue is not fixed, right? |
Ah, I thought I had fixed that but now I did for real. |
|
Performance compared to mut-treelist before: After: |
|
This is ready to merge, right? |
|
From my perspective yes although I was hoping for a review from @rmculpepper |
rmculpepper
left a comment
rmculpepper
left a comment
There was a problem hiding this comment.
Gvectors were previously unsynchronized and so not thread-safe. The introduction of unsafe operations makes them potentially memory-unsafe, and we need to make sure gvector operations, even used concurrently, are still memory-safe.
The gv-ensure-space! pattern helps: it means that writes to the vector have enough space, whether or not that vector is actually installed as the gvector's vec field at the end of the operation (eg, because of two racing add operations). I'm worried about the write to n afterwards; in a race, it might point past the end of vec.
I've pointed out a problem in gvector-ref if there is a concurrent call that shrinks the gvector. It also occurs if n is greater than the length of the vector. Other read operations (iteration, for example) might have similar problems, but I haven't checked them all.
If unsafe operations are used, we need to figure out and document invariants and patterns of field updates that actually preserve memory-safety.
Based on the benchmark from PR racket#34, compares gvector operations against mutable treelists across varying vector counts and lengths.
0e81c30 to
52cf25b
Compare
|
@rmculpepper, can you re-review this? |
|
I think it's still unsafe. Suppose Here are two candidate invariants:
I think (2) holds, but |
Replace contracts with inline checks and unsafe operations for gvector-ref, gvector-set!, gvector-add!, and in-gvector iteration. Use CAS on vec field and vec-before-n read ordering for memory safety under concurrent access. Add parallel/concurrent stress tests.
|
I revised it to ensure both invariants and to use There's a stress test (written by Claude) which triggers all the problematic issues by adding sleep inside various operations. After review, I will take those out before we merge. |
|
These changes do make it slightly slower than my original code, but still much faster than the code in the tree now. |
|
ping @rmculpepper |
Unsafe operations on the paths every push, pop and indexed access goes
through. Each use rests on an invariant the library already maintains: a
chunk's backing vector is allocated here and never impersonated; an index into
it is always reduced modulo the capacity; and heads, sizes and weights are
bounded by a vector or sequence length, so they are fixnums. Indexed access
to a short ephemeral sequence goes from 17.8ns to 7.6ns, a random write from
20.3ns to 10.4ns.
Two deviations from the reference are closed, since neither was buying
anything:
* flatten now consumes an ephemeral argument and its elements, as the
reference's does. It is also the faster way to do it, since it hands over
each sequence's representation rather than copying its elements.
* reach now scans an unpacked chunk from the cursor's position rather than
from the chunk's start, which was the remaining piece of the reference's
locality optimization.
The third documented deviation stays, because the benchmarks show it is an
optimization and not merely a difference: sharing the end chunks across edit
and snapshot rather than copying them makes a snapshot-after-every-push loop
nine times faster than the reference, at the cost of being 2.8x slower for a
single snapshot in isolation.
Two things the benchmarks turned up, now fixed:
* in-sek, in-pseq and in-eseq did not participate in for's fast path, so a
loop over them cost 18x what a loop over in-gvector cost. They are now
sequence syntaxes that expand to a loop over the sequence's own storage.
for/eseq, for*/eseq, for/pseq and for*/pseq come with them.
* An ephemeral sequence eagerly allocated its two end chunks, so creating a
million ten-element sequences allocated a million chunks of capacity 128.
The allocation is now deferred to the first push to that side.
New benchmark scenarios for transience, which the reference's own suite does
not measure -- it uses snapshot and edit only to set sequences up. Racket's
mutable-treelist is the comparison, since treelist-copy and
mutable-treelist-snapshot are the same pair of conversions. They are O(n)
each, where sek's are not: one snapshot of a million elements costs 1.7ms
against 209ns, and a round trip that changes one element between snapshots is
four orders of magnitude apart. Also a filter scenario, section 5.4's
motivating example for iterators.
bench/bm.rkt is the benchmark from racket/data#34 with sek rows added, run in
its terms. It says something main.rkt does not: at a thousand sequences of
ten elements sek loses at everything, because a chunk of capacity 128 is a bad
container for ten elements, and at a hundred sequences of a hundred thousand
it wins at everything except indexing. The gvector measured throughout is the
revised one from that PR.
4 participants
single-value performance (see Optimizations and benchmarks for gvectors racket#4942).
(taken from Rust's Vector implementation).
vector-extendfrom Addvector-extend. racket#4943.