refactor!(core): change underlying implementation of `CMap2` to add sync mechanisms #201

imrn99 · 2024-10-17T12:17:52Z

remove the Clone trait impl when feature utils was enabled (unused after some benchmark deletion)
comment out AttrCompactVec for now
replace internal beta storage type u32 by its atomic equivalent AtomicU32
replace unused darts BTreeSetby Vec<AtomicBool>
change AttrSparseVec internal storage type Option<A> by Atomic<Option<A>> using the atomic crate
Update signatures from CMap2, storage traits methods, storage manager, to be non-mutable
add Send & Sync implementations for:
- AttrSparseVec, AttrStorageManager
- CMap2
- VertexCollection, EdgeCollection, FaceCollection
- Vertex2, Vector2

codecov-commenter · 2024-10-17T12:20:18Z

Codecov Report

Attention: Patch coverage is 28.45238% with 601 lines in your changes missing coverage. Please review.

Project coverage is 74.45%. Comparing base (fdad996) to head (d2ea11d).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
honeycomb-core/src/attributes/manager.rs	7.78%	237 Missing ⚠️
honeycomb-core/src/cmap/dim2/link_and_sew.rs	20.22%	217 Missing ⚠️
honeycomb-core/src/cmap/dim2/basic_ops.rs	33.00%	67 Missing ⚠️
honeycomb-core/src/attributes/collections.rs	63.69%	57 Missing ⚠️
honeycomb-core/src/attributes/traits.rs	0.00%	10 Missing ⚠️
honeycomb-core/src/cmap/dim2/utils.rs	20.00%	8 Missing ⚠️
honeycomb-core/src/cmap/dim2/embed.rs	57.14%	3 Missing ⚠️
honeycomb-core/src/cmap/dim2/structure.rs	90.90%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   82.12%   74.45%   -7.68%     
==========================================
  Files          41       41              
  Lines        5650     6173     +523     
==========================================
- Hits         4640     4596      -44     
- Misses       1010     1577     +567

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

imrn99 · 2024-10-18T06:51:05Z

`Clone` impl / `AttrCompactVec` removal

this is a convenience change. both weren't used currently and require significant changes to make them work in parallel. this can be done in a separate PR

imrn99 · 2024-10-18T07:01:15Z

Internal beta storage

This is the storage structure of beta functions:

values were stored using u32 (through an alias named DartIdentifier). the internal type was changed to AtomicU32, while method & function signatures still take DartIdentifiers as arguments.

atomic load/write is handled internally, with no change to the public API (aside from mutability requirement). all operations on these are using a relaxed ordering, but this is subject to change.

the external vector (red) is not wrapped in a synchronization layer as we consider that expending, reallocating, etc. should be done before heavily-parallel computation kernels. if, at any point, we need to do this, the mechanism used should probably be a RwLock.

imrn99 · 2024-10-18T07:16:11Z

unused darts tracking

reminder: dart removal means (a) set its beta values to NULL_DART_ID (== 0), and (b) mark it as unused

if we take the last step of the clipping routine of the grisubal algorithm as an example, the problem with the original data structure is obvious: after a coloring algorithm, we mass-delete darts that are not part of final structure.

if we were to parallelize this mass-deletion step, all threads would be contending over access to a single BTreeSet, to insert the dart being deleted. by replacing the set with a vector of transactional variables, we can ensure that there won't be contention over this operation, assuming that two threads do not attempt to delete the same dart (which is an incorrect operation anyway)

the vector itself is not wrapped in a sync layer for the same reason mentionned in the beta function storage:

the external vector (red) is not wrapped in a synchronization layer as we consider that expending, reallocating, etc. should be done before heavily-parallel computation kernels. if, at any point, we need to do this, the mechanism used should probably be a RwLock.

imrn99 · 2024-10-18T08:09:46Z

vertex / attribute storage

the issue is illustrated here:

these storages must satisfy stronger synchronization requirements. because there is an intermediate state where not all of the involved data has been attributed its final values, we need to ensure nothing can happen in that time frame for operation correctness.

a first idea was to wrap each entry in RwLocks. while the implementation worked, it was discarded for something simpler that should work (needs testing using something like loom).

a second idea was to use atomic orderings to avoid inconsistencies. it was dropped because it actually does not work.

the solution is to wrap attribute values in atomics, and use Acquire/Release/AcqRel ordering to ensure there's no >problematic instruction re-ordering. the idea came from @dssgabriel, and we speculated that these orderings are sufficient >for the operation described.

the merge operation represented above is implemented like this:

...

the Acq loads should prevent other operations to occur before the final Rel stores.~

the adopted solution is to use the transactional memory model (see comment below)

imrn99 · 2024-10-23T12:41:55Z

following a discussion with @cedricchevalier19, the attribute storage synchronization mechanism has been switch to use a software transactional memory (STM) implementation. the model is implemented by the stm crate, which hasn't received an update in a long time, but still has clean and concise code.

// create the scope of the transaction
atomically(|trans| {
            let new_v = match (
                self.data[lhs_inp as usize].read(trans)?, // all ops on variable take a reference to the transaction as arg
                self.data[rhs_inp as usize].read(trans)?, // all `?` call are used to catch invalid transaction 
            ) {
                (Some(v1), Some(v2)) => Some(AttributeUpdate::merge(v1, v2)),
                (Some(v), None) | (None, Some(v)) => Some(AttributeUpdate::merge_incomplete(v)),
                (None, None) => AttributeUpdate::merge_from_none(),
            };
            if new_v.is_none() {
                eprintln!("W: cannot merge two null attribute value");
                eprintln!("   setting new target value to `None`");
            }
            self.data[rhs_inp as usize].write(trans, None)?;
            self.data[lhs_inp as usize].write(trans, None)?;
            self.data[out as usize].write(trans, new_v)?;
            Ok(())
});

cedricchevalier19

The mix between Atomic and Transactions is hard to follow. Can you explain what has led to these choices?

cedricchevalier19 · 2024-10-24T12:18:13Z

honeycomb-core/src/attributes/collections.rs

+                eprintln!("   setting both new values to `None`");
+                self.data[lhs_out as usize].write(trans, None)?;
+                self.data[rhs_out as usize].write(trans, None)?;
+                //self.data[inp as usize].store(None, Ordering::Release);


I believe that if the original attribute undefined, this should be cascaded to both new attributes.

Aside from that, I think that both output values should already be undefined (making these two lines useless) because of the design of my indexing logic. I'm not 100% sure and I don't want to rely on this without proving it first (which can probably be done).

cedricchevalier19 · 2024-10-24T13:16:10Z

honeycomb-core/src/attributes/tests.rs

    generate_manager!(manager);
-    generate_compact!(storage);
-    assert_eq!(storage.remove(3), Some(Temperature::from(279.0)));
+    // what was this ?


Good question!

cedricchevalier19 · 2024-10-24T13:19:41Z

honeycomb-core/src/cmap/dim2/basic_ops.rs


 use std::collections::BTreeSet;
-
+use std::sync::atomic::{AtomicBool, Ordering};


Why do not use transactions here too?

Ok, there are two things:

for unused dart storage, AtomicBool is enough because there's no situation / operation where the validity of a dart is a critical information. This could change if we implement algorithms that delete part of the mesh sporadically, but even then it doesn't matter: What is important is that all beta function of the deleted dart are set to 0, not that it is marked. marks matter when fetching all cells after (or before) an algorithm is executed.

for beta values: these should probably be transactional too. the biggest consequence would be that some method (e.g. attribute collection methods) need to take a transaction as argument as the atomically section provided byt he stm crate should not be nested

imrn99 · 2024-10-28T07:25:32Z

I rewrote a few examples on a white board and found cases where atomics on beta values were not satisfying. given this, I changed the storage type to TVar<DartIdentifier>. I reimplemented methods as needed to avoid nesting transaction in higher level ops (e.g. sew). this was made easy by the stm crate as nested transaction result in a crash at runtime.

these changes significantly enlarge the PR, but some cleanup is possible. as of now, I duplicated most original methods to add variants taking a Transaction as argument. it might be better to replace the original methods, although it will absolutely break the API

the most notable changes (that adds a lot of code) is the computation of new IDs in sew/unsew methods. the issue is that the ID must be computed with the edited beta values (i.e. the ones of the transaction), but adding a reference to a transaction in an Orbit2 significantly complexifies the code. I decided to hardcode this ID computation in new methods: vertex_id_transac, edge_id_transac, face_id_transac. this is simpler and clearer.

cedricchevalier19 · 2024-10-28T13:19:05Z

Ok, I will take my time to understand the changes.

imrn99 · 2024-10-30T07:07:57Z

following a conversation with @cedricchevalier19, this is the course of action we'll follow:

switch unused_darts storage to transactional variables too, in order to fully adopt the model and its guarantees
merge this PR
write tests and merge them in a separate PR
cleanup in a separate PR

imrn99 added 2 commits October 17, 2024 11:58

replace derive(Clone) with manual impl to refactor more easily

44eee1f

replace internal beta storage type by AtomicU32

567288a

imrn99 added 3 commits October 17, 2024 14:30

replace unused_darts set by Vec<AtomicBool>

39d7d74

fix a lint

e98de49

update method sigs

dc9dc1d

This was referenced Oct 17, 2024

enh: crate parallelization #197

Closed

feat(core): implement parallel variants of attribute storage traits & structs #199

Closed

imrn99 added 5 commits October 17, 2024 15:00

remove useless Clone impl

38a7e9d

add sync mechanism to AttrSparseVec using atomics

5471d84

update storage trait signatures & attribute manager

dd6881f

update CMap2 method sigs

8ce2a80

fix lints

0abe347

imrn99 marked this pull request as ready for review October 18, 2024 08:09

imrn99 requested review from cedricchevalier19 and franck-ledoux October 18, 2024 08:10

add Send/Sync impl to relevant items

2ac57d8

imrn99 changed the title ~~refactor(core): change underlying implementation of CMap2 to add sync mechanisms~~ refactor!(core): change underlying implementation of CMap2 to add sync mechanisms Oct 18, 2024

imrn99 added 2 commits October 18, 2024 11:36

add add missing Send/Sync reqs

18045dd

Merge branch 'master' into cmap-parallel

591cf49

imrn99 mentioned this pull request Oct 21, 2024

feat(bench): add fetch_... methods benchmarks #202

Merged

imrn99 added 3 commits October 22, 2024 15:43

Merge branch 'master' into cmap-parallel

1f32789

replace atomics with transactional memory in attribute storage impl

030afd7

remove unused deps

22f30f7

cedricchevalier19 reviewed Oct 24, 2024

View reviewed changes

imrn99 mentioned this pull request Oct 24, 2024

feat(core): add struct wrappers around inner storages #208

Merged

imrn99 added 8 commits October 25, 2024 08:58

switch beta values type to TVar

0b5c87f

allow unused_mut in tests

38f4381

xreate link core methods to expose the transaction arg

54d1617

repeat for attribute storage

ad5a4b2

expose transactionnal method in traits & manager

38edf34

fix doc

47e5581

implement (un)sews stm variants

50aef66

add lint exception

d2ea11d

imrn99 requested a review from cedricchevalier19 October 28, 2024 07:19

swap AtomicBool for TVar<bool>

5940066

imrn99 merged commit e179335 into master Oct 30, 2024
1 of 13 checks passed

imrn99 deleted the cmap-parallel branch October 30, 2024 07:33

This was referenced Oct 30, 2024

test(core): add coverage over new parallel implementations #214

Merged

enh: mitigate regression induced by the parallel implementation #215

Closed


		use std::collections::BTreeSet;

		use std::sync::atomic::{AtomicBool, Ordering};

refactor!(core): change underlying implementation of CMap2 to add sync mechanisms #201

refactor!(core): change underlying implementation of CMap2 to add sync mechanisms #201

Uh oh!

Conversation

imrn99 commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

imrn99 commented Oct 18, 2024

Clone impl / AttrCompactVec removal

Uh oh!

imrn99 commented Oct 18, 2024

Internal beta storage

Uh oh!

imrn99 commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

unused darts tracking

Uh oh!

imrn99 commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

vertex / attribute storage

Uh oh!

imrn99 commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cedricchevalier19 left a comment

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

imrn99 Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

cedricchevalier19 Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

imrn99 Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imrn99 commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cedricchevalier19 commented Oct 28, 2024

Uh oh!

imrn99 commented Oct 30, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

refactor!(core): change underlying implementation of `CMap2` to add sync mechanisms #201

refactor!(core): change underlying implementation of `CMap2` to add sync mechanisms #201

imrn99 commented Oct 17, 2024 •

edited

Loading

codecov-commenter commented Oct 17, 2024 •

edited

Loading

`Clone` impl / `AttrCompactVec` removal

imrn99 commented Oct 18, 2024 •

edited

Loading

imrn99 commented Oct 18, 2024 •

edited

Loading

imrn99 commented Oct 23, 2024 •

edited

Loading

imrn99 Oct 25, 2024 •

edited

Loading

imrn99 commented Oct 28, 2024 •

edited

Loading