First implementation of new SI API #2

TedHartMS · 2021-02-17T22:57:36Z

No description provided.

badrishc · 2021-02-18T00:01:38Z

cs/benchmark/SecondaryIndexes.cs

+
+        public void Delete(ref Key key) { }
+
+        public void Insert(ref Key key) { }


There is a variant here that takes in the address as well. In that case, we need to update the index whenever addresses changes, even if the key does not change. Like an Upsert call here in addition to Insert.

badrishc · 2021-02-18T00:02:06Z

cs/benchmark/SecondaryIndexes.cs

+
+        public void Insert(ref Value value, long recordId) { }
+
+        public void Upsert(ref Value value, long recordId, bool isMutable) { }


What does isMutable mean here in the API?

badrishc · 2021-02-18T00:02:36Z

cs/src/core/Allocator/GenericAllocator.cs

@@ -922,7 +922,7 @@ protected override bool RetrievedFullRecord(byte* record, ref AsyncIOContext<Key
        /// <returns></returns>
        public override bool KeyHasObjects()
        {
-            return SerializerSettings.keySerializer != null;
+            return SerializerSettings?.keySerializer != null;


Check where the nullref occurred and try to fix the caller spot.

badrishc · 2021-02-18T00:04:03Z

cs/src/core/ClientSession/AdvancedClientSession.cs

@@ -737,15 +737,54 @@ public void ConcurrentReader(ref Key key, ref Input input, ref Value value, ref

            public bool ConcurrentWriter(ref Key key, ref Value src, ref Value dst, long address)
            {
-                return _clientSession.functions.ConcurrentWriter(ref key, ref src, ref dst, address);
+                if (!_clientSession.fht.SupportsMutableIndexes)


Make SupportsMutableIndexes a field of this struct and see if it helps.

Other thing to try is to make this struct itself readonly -- will that help.

Moving else path to non-inlined extra heavy-method might help the fast path?

badrishc · 2021-02-18T00:13:38Z

cs/src/core/ClientSession/ClientSession.cs

+                if (!_clientSession.fht.SupportsMutableIndexes)
+                    return _clientSession.functions.ConcurrentWriter(ref key, ref src, ref dst);
+
+                ref RecordInfo recordInfo = ref _clientSession.fht.RecordAccessor.SpinLockRecordInfo(address);


It's a bit worrisome that locking is all over the place and using spin locks.

Scenarios to consider:

What if the value were 8 byte longs and so we didn't need a lock at all?

Or the user is okay with broken writes (value consists of individual 8-byte values that don't all need to be consistently read at ones)

What if we were only having key indexes, so we never need to take a lock at all (since keys are immutable)?

What if the user had space in key/value to have a better implementation of lock.

Would it be better if we made this a duty of (advanced) functions - capable of being overridden by specific functions. like functions.LockRecord(ref RecordInfo, ref Key, ref Value).

badrishc · 2021-02-18T00:15:15Z

cs/src/core/ClientSession/ClientSession.cs

+                {
+                    if (!recordInfo.Tombstone && _clientSession.functions.ConcurrentWriter(ref key, ref src, ref dst))
+                    {
+                        // KeyIndexes do not need notification of in-place updates because the key does not change.


The lock seems wasteful to take and release if we didnt need to notify SI.

badrishc · 2021-02-18T00:16:20Z

cs/src/core/ClientSession/ClientSession.cs

+                if (!_clientSession.fht.SupportsMutableIndexes)
+                    return false;
+
+                ref RecordInfo recordInfo = ref _clientSession.fht.RecordAccessor.SpinLockRecordInfo(address);


Again, we are taking a lock even when we do not need to talk to SI.

badrishc · 2021-02-18T00:25:23Z

cs/src/core/SecondaryIndex/SecondaryIndexBroker.cs

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void Insert(ref TKVKey key)
+        {
+            foreach (var keyIndex in mutableKeyIndexes)


Is this iteration thread-safe with adding indexes?

badrishc · 2021-02-18T00:27:36Z

cs/src/core/ClientSession/ClientSession.cs

+                    if (!recordInfo.Tombstone && _clientSession.functions.ConcurrentWriter(ref key, ref src, ref dst))
+                    {
+                        // KeyIndexes do not need notification of in-place updates because the key does not change.
+                        if (_clientSession.fht.SecondaryIndexBroker.MutableValueIndexCount > 0)


Keys in FASTER are immutable once added to the log, so we do not need a lock when only accessing keys.

…nctionsBase

badrishc · 2021-02-24T03:28:07Z

cs/src/core/VarLen/SpanByteFunctions.cs

-        {
-            this.locking = locking;
-        }
+        public SpanByteFunctions(bool locking = false) : base(locking) { }


Why are we still doing locking inside ConcurrentWriter below? Shouldn't it be refactored into Lock() and Unlock() overrides.

badrishc · 2021-02-24T03:29:10Z

cs/src/core/VarLen/MemoryFunctions.cs


        /// <summary>
        /// Constructor
        /// </summary>
        /// <param name="memoryPool"></param>
        /// <param name="locking">Whether we lock values before concurrent operations (implemented using a spin lock on length header bit)</param>
-        public MemoryFunctions(MemoryPool<T> memoryPool = default, bool locking = false)
+        public MemoryFunctions(MemoryPool<T> memoryPool = default, bool locking = false) : base(locking)


Why are we still doing locking inside ConcurrentWriter below? Shouldn't it be refactored into Lock() and Unlock() overrides.

badrishc · 2021-02-24T03:32:35Z

cs/test/ObjectTestTypes.cs

@@ -328,13 +281,13 @@ public class MyLargeOutput
        public MyLargeValue value;
    }

-    public class MyLargeFunctions : IFunctions<MyKey, MyLargeValue, MyInput, MyLargeOutput, Empty>
+    public class MyLargeFunctions : FunctionsBase<MyKey, MyLargeValue, MyInput, MyLargeOutput, Empty>


It would be good practice to seal all overridden user functions.

badrishc · 2021-02-24T03:33:39Z

cs/src/core/SecondaryIndex/SecondaryIndexBroker.cs

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void Delete(long recordId)
+        {
+            foreach (var valueIndex in mutableValueIndexes)


if foreach as efficient in assembly code as a normal for loop?

badrishc · 2021-02-24T03:36:56Z

cs/src/core/Index/Common/Contexts.cs

        UPSERT,
+        /// <summary>Insert operation (either Upsert or RMW resulted in a new record)</summary>


Is INSERT also used in internal use of OperationType. Why not keep a separate enum for the user facing API compared to internal?

badrishc · 2021-02-24T03:39:29Z

cs/src/core/ClientSession/AdvancedClientSession.cs

            {
-                return _clientSession.functions.InPlaceUpdater(ref key, ref input, ref value, address);
+                long context = 0;
+                this.Lock(ref recordInfo, ref key, ref value, OperationType.RMW, ref context);


It seems overkill to send OperationType to Lock. All we need to know here is whether this is a reader lock or writer lock. That is why I suggested either a new enum OpType or just a bool.

Maybe call it LockType.Shared and Exclusive?

- enable locking in YCSB benchmark - re-allow threadAffinitized sessions with indexes (still need to add some SI calls in pending handlers) - rename to .SupportsLocking - create LockType enum rather than reusing OperationType - make tests for context.IsNewRecord more efficient in ContextUpsert et al.

… add an iteration count; refactor Memory and Span Functions to use the new locking scheme

…files, validate params, generate parsable output

- Force synthetic data for kUseSmallData - Add the Zipf generator for synthetic data - Improve output formatting so it is more easily parsable and has all needed info

…pare results

… scenarios

…Id from Key Index and split out the Key vs. Value index test base classes; make IHeapContainer inherit from IDisposable; add SI calls to UpsertAsync

…nternal; Fix QueryRecord.Dispose(); make RecordId.IsDefault() a method so it's not serialized

- Add RIDs to Key indexes - Add TKVKEey to Value indexes - Add Checkpoint/Restore API - Add Truncation API

First implementation of new SI API

3aa431d

TedHartMS requested a review from badrishc February 17, 2021 22:57

badrishc reviewed Feb 18, 2021

View reviewed changes

TedHartMS added 4 commits February 19, 2021 13:26

Add I(Advanced)Functions.Lock/Unlock and minimize SI-related locking

97bcf90

some experimental stuff to test

1b10d64

Experiment with SupportsLocks

d9db397

Change to new locking scheme; update test Functions to derive from Fu…

c54018c

…nctionsBase

badrishc reviewed Feb 24, 2021

View reviewed changes

TedHartMS and others added 9 commits February 24, 2021 21:51

Update YCSB benchmark to separate locking and indexing parameters and…

85c54a4

… add an iteration count; refactor Memory and Span Functions to use the new locking scheme

updates

11c0289

Improvments to multi-iteration runs

ac12836

Remove FKV.SupportsMutableIndexes; change benchmark to resuse loaded …

2c94bd7

…files, validate params, generate parsable output

Benchmark: remove interface and make store, device, keys readonly

e340da7

distinct delimiter on last and trimmed stats summary

a094f81

More YCSB benchmark updates:

3b118b8

- Force synthetic data for kUseSmallData - Add the Zipf generator for synthetic data - Improve output formatting so it is more easily parsable and has all needed info

Update benchmark to start in unison.

ab625ac

TedHartMS added 30 commits March 8, 2021 21:09

YCSB: change --backup to --recover and remove BackupMode; add --runsec

eb59000

a few minor tweaks made while doing PRs

664e7dc

Add Powershell scripts to run the benchmark performance suite and com…

d15c6c6

…pare results

Merge upstream master

3bb74ad

updates to benchmark scripts

ef80838

Merge remote-tracking branch 'origin/master' into SecondaryIndex

b12d4d6

Add SubsetIndexSessionBroker; add SimpleIndex tests

2ce51aa

Implement ReadOnlyObserver for SI; add Immutable and Mixed index test…

c179dc8

… scenarios

Support multiple ReadOnly and Eviction Observers

42619d7

Merge upstream master

25730b9

more files from merge

01220cf

Merge remote-tracking branch 'origin/master' into SecondaryIndex

e30da07

Add RecordId struct; move QueryRecord into SI from HVI; remove record…

ab1c815

…Id from Key Index and split out the Key vs. Value index test base classes; make IHeapContainer inherit from IDisposable; add SI calls to UpsertAsync

Add QueryRecord.RecordId

8c88c1a

RecordId changes from HVI

3b57ef1

QueryRecord changes from HVI

f024f3a

Fixes to multi-observer cleanup; fix recordInfo setting in ReadAsyncI…

074028b

…nternal; Fix QueryRecord.Dispose(); make RecordId.IsDefault() a method so it's not serialized

merge upstream master

5bf46d9

Modify Secondary Index interfaces:

4458616

- Add RIDs to Key indexes - Add TKVKEey to Value indexes - Add Checkpoint/Restore API - Add Truncation API

Merge upstream master

881f4bd

Merge upstream master; backport from HVI branch

32320de

Merge remote-tracking branch 'origin/master' into SecondaryIndex

f6a9603

allow deltaFileDevice == null

101aff6

Additional changes for C#9

8a321d9

Backport a few changes from HVI in preparation to merge master

d380af8

Merge upstream master

5a40baa

merge upstream master

a2a55ac

add missing OnRecovery to InternalRecoverAsync

7917538

Add default values for scanDelta, recoverTo args

25f310f

backport mostly doc-related changes from HVI

c4b0781

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First implementation of new SI API #2

First implementation of new SI API #2

TedHartMS commented Feb 17, 2021

badrishc Feb 18, 2021

badrishc Feb 18, 2021

badrishc Feb 18, 2021

badrishc Feb 18, 2021

badrishc Feb 18, 2021 •

edited

Loading

badrishc Feb 18, 2021

badrishc Feb 18, 2021

badrishc Feb 18, 2021

badrishc Feb 18, 2021

badrishc Feb 24, 2021

badrishc Feb 24, 2021

badrishc Feb 24, 2021

badrishc Feb 24, 2021

badrishc Feb 24, 2021

badrishc Feb 24, 2021

badrishc Feb 24, 2021


		public void Delete(ref Key key) { }

		public void Insert(ref Key key) { }


		public void Insert(ref Value value, long recordId) { }

		public void Upsert(ref Value value, long recordId, bool isMutable) { }

		UPSERT,
		/// <summary>Insert operation (either Upsert or RMW resulted in a new record)</summary>

First implementation of new SI API #2

Are you sure you want to change the base?

First implementation of new SI API #2

Conversation

TedHartMS commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badrishc Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badrishc Feb 18, 2021 •

edited

Loading