From 14aea612d6e648921ad5fd43ec49ef4e27ff456d Mon Sep 17 00:00:00 2001 From: Marcus Denker Date: Wed, 6 Dec 2023 14:15:45 +0100 Subject: [PATCH] This PR removes the feature that SoilIndexedDictionary can be created without a transaction and used - remove the code that checked for nil - remove the code that transformed new values when a transaction is set - Fix tests - remove tests that where using the dict without a transaction The newValues are now still there, but only used to create the transaction log and #hasIndexUpdates. --- src/Soil-Core-Tests/SoilBackupTest.class.st | 12 +- .../SoilIndexedDictionaryTest.class.st | 184 ++---------------- src/Soil-Core/SoilIndexedDictionary.class.st | 175 ++++++----------- 3 files changed, 77 insertions(+), 294 deletions(-) diff --git a/src/Soil-Core-Tests/SoilBackupTest.class.st b/src/Soil-Core-Tests/SoilBackupTest.class.st index 54da88fd..86be5603 100644 --- a/src/Soil-Core-Tests/SoilBackupTest.class.st +++ b/src/Soil-Core-Tests/SoilBackupTest.class.st @@ -39,10 +39,13 @@ SoilBackupTest >> testBackupWithIndex [ dict := SoilSkipListDictionary new keySize: 8; maxLevel: 16. + + tx := soil newTransaction. + tx root: dict. + dict at: #foo put: (SoilTestNestedObject new label: #indexed). object := SoilTestClusterRoot new nested: dict. - tx := soil newTransaction. tx root: dict. tx commit. soil backupTo: self backupPath. @@ -60,15 +63,18 @@ SoilBackupTest >> testBackupWithIndexRemoval [ | tx backup tx2 dict object | "removed keys in indexes get objectId 0:0. On backup time we only need to copy the non-removed" + dict := SoilSkipListDictionary new keySize: 8; maxLevel: 16. + tx := soil newTransaction. + tx root: dict. + dict at: #foo put: (SoilTestNestedObject new label: #indexed). dict at: #bar put: (SoilTestNestedObject new label: #bar). object := SoilTestClusterRoot new nested: dict. - tx := soil newTransaction. - tx root: dict. + tx commit. tx2 := soil newTransaction. tx2 root removeKey: #bar. diff --git a/src/Soil-Core-Tests/SoilIndexedDictionaryTest.class.st b/src/Soil-Core-Tests/SoilIndexedDictionaryTest.class.st index 011ccbb2..913cda4e 100644 --- a/src/Soil-Core-Tests/SoilIndexedDictionaryTest.class.st +++ b/src/Soil-Core-Tests/SoilIndexedDictionaryTest.class.st @@ -74,16 +74,6 @@ SoilIndexedDictionaryTest >> testAddAndRemoveExistingList [ ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testAddAndRemoveOnNewList [ - self - shouldnt: [ dict at: #foo put: #bar ] - raise: Error. - self assert: (dict at: #foo) equals: #bar. - dict removeKey: #foo. - self assert: dict size equals: 0 -] - { #category : #tests } SoilIndexedDictionaryTest >> testAddToExistingEmptyList [ | tx tx2 tx3 tx4 | @@ -139,31 +129,6 @@ SoilIndexedDictionaryTest >> testAddToExistingNonEmptyList [ self assert: (tx4 root at: #foo) equals: #bar. ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testAddToNewList [ - self - shouldnt: [ dict at: #foo put: #bar ] - raise: Error. - self assert: (dict at: #foo) equals: #bar -] - -{ #category : #tests } -SoilIndexedDictionaryTest >> testAt [ - dict at: #foo2 put: #bar2. - dict at: #foo put: #bar. - - self assert: (dict at: #foo2) equals: #bar2. - self should: [dict at: #ff] raise: KeyNotFound -] - -{ #category : #tests } -SoilIndexedDictionaryTest >> testAtIndex [ - dict at: #foo2 put: #bar2. - dict at: #foo put: #bar. - - self assert: (dict atIndex: 1) equals: #bar2 -] - { #category : #tests } SoilIndexedDictionaryTest >> testAtIndexWithTransaction [ | tx tx2 | @@ -183,9 +148,8 @@ SoilIndexedDictionaryTest >> testAtIndexWithTransaction [ SoilIndexedDictionaryTest >> testConcurrentAddKey [ | tx1 tx2 tx3 | tx1 := soil newTransaction. - tx1 root: (dict - at: #one put: #onevalue; - yourself). + tx1 root: dict. + dict at: #one put: #onevalue. tx1 commit. tx2 := soil newTransaction. "After creating tx2 we open a concurrent transaction and add a key to @@ -202,12 +166,12 @@ SoilIndexedDictionaryTest >> testConcurrentAddKey [ SoilIndexedDictionaryTest >> testConcurrentDo [ | tx1 tx2 tx3 col | tx1 := soil newTransaction. - tx1 root: (dict + tx1 root: dict. + dict at: #one put: #onevalue; at: #two put: #twovalue; at: #three put: #threevalue; - at: #four put: #fourvalue; - yourself). + at: #four put: #fourvalue. tx1 commit. tx2 := soil newTransaction. "After creating tx2 we open a concurrent transaction and add a key to @@ -227,9 +191,10 @@ SoilIndexedDictionaryTest >> testConcurrentDo [ SoilIndexedDictionaryTest >> testConcurrentIsEmpty [ | tx1 tx2 tx3 | tx1 := soil newTransaction. - tx1 root: (dict - at: #one put: #onevalue; - yourself). + tx1 root: dict. + dict + at: #one + put: #onevalue. tx1 commit. tx2 := soil newTransaction. "After creating tx2 we open a concurrent transaction and add a key to @@ -248,9 +213,10 @@ SoilIndexedDictionaryTest >> testConcurrentIsEmpty [ SoilIndexedDictionaryTest >> testConcurrentRemoveKey [ | tx1 tx2 tx3 | tx1 := soil newTransaction. - tx1 root: (dict - at: #one put: #onevalue; - yourself). + tx1 root: dict. + dict + at: #one + put: #onevalue. tx1 commit. tx2 := soil newTransaction. "After creating tx2 we open a concurrent transaction and remove a key to @@ -264,19 +230,6 @@ SoilIndexedDictionaryTest >> testConcurrentRemoveKey [ ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testDo [ - | counter | - dict at: #foo2 put: #bar2. - dict at: #foo put: #bar. - - counter := 0. - dict do: [ :each | - self assert: (each beginsWith: 'bar'). - counter := counter + 1]. - self assert: counter equals: 2 -] - { #category : #tests } SoilIndexedDictionaryTest >> testDoWithTransAction [ | tx tx1 tx2 counter | @@ -315,33 +268,6 @@ SoilIndexedDictionaryTest >> testDoWithTransAction [ ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testFirst [ - dict at: #foo2 put: #bar2. - dict at: #foo put: #bar. - - "first in key order" - self assert: dict first equals: #bar. - self assert: (dict first: 1) first equals: #bar. -] - -{ #category : #tests } -SoilIndexedDictionaryTest >> testFirstAssociation [ - dict at: #foo2 put: #bar2. - dict at: #foo put: #bar. - - "firstAssocation in key order" - self assert: dict firstAssociation equals: #foo->#bar. -] - -{ #category : #tests } -SoilIndexedDictionaryTest >> testFirstAssociationWithSingleRemovedItem [ - - dict at: #foo put: #bar. - dict removeKey: #foo. - self assert: dict firstAssociation equals: nil -] - { #category : #tests } SoilIndexedDictionaryTest >> testFirstAssociationWithTransaction [ | tx tx2 | @@ -358,14 +284,6 @@ SoilIndexedDictionaryTest >> testFirstAssociationWithTransaction [ ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testFirstWithSingleRemovedItem [ - - dict at: #foo put: #bar. - dict removeKey: #foo. - self assert: dict first equals: nil -] - { #category : #tests } SoilIndexedDictionaryTest >> testFirstWithTransaction [ | tx tx2 | @@ -467,31 +385,6 @@ SoilIndexedDictionaryTest >> testIsEmpty [ self deny: tx1 root isEmpty. ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testLast [ - dict at: #foo2 put: #bar2. - dict at: #foo put: #bar. - - "last in key order" - self assert: dict last equals: #bar2 -] - -{ #category : #tests } -SoilIndexedDictionaryTest >> testLastAssociation [ - dict at: #foo2 put: #bar2. - dict at: #foo put: #bar. - - "last association in key order" - self assert: dict lastAssociation equals: #foo2->#bar2 -] - -{ #category : #tests } -SoilIndexedDictionaryTest >> testLastAssociationWithSingleRemovedItem [ - dict at: #foo put: #bar. - dict removeKey: #foo. - self assert: dict lastAssociation equals: nil -] - { #category : #tests } SoilIndexedDictionaryTest >> testLastAssociationWithTransaction [ | tx tx2 | @@ -508,13 +401,6 @@ SoilIndexedDictionaryTest >> testLastAssociationWithTransaction [ self assert: tx2 root lastAssociation equals: 2->#two ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testLastWithSingleRemovedItem [ - dict at: #foo put: #bar. - dict removeKey: #foo. - self assert: dict last equals: nil -] - { #category : #tests } SoilIndexedDictionaryTest >> testLastWithTransaction [ | tx tx2 | @@ -547,14 +433,6 @@ SoilIndexedDictionaryTest >> testLastWithTransactionRemoveLast [ self assert: tx2 root last equals: #one ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testNextAfter [ - dict at: 1 put: #bar. - dict at: 2 put: #bar2. - - self assert:( dict nextAfter: 1) equals: #bar2 -] - { #category : #tests } SoilIndexedDictionaryTest >> testNextAfterWithTransaction [ | tx tx2 | @@ -570,17 +448,6 @@ SoilIndexedDictionaryTest >> testNextAfterWithTransaction [ self assert: (tx2 root nextAfter: 1) value equals: #two ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testRemoveKey [ - dict at: #foo put: #bar. - dict at: #foo2 put: #bar2. - - dict removeKey: #foo. - self assert: dict size equals: 1. - - self should: [ dict removeKey: #blah ] raise: KeyNotFound -] - { #category : #tests } SoilIndexedDictionaryTest >> testRemoveKeyIfAbsentWithTransaction [ @@ -636,14 +503,6 @@ SoilIndexedDictionaryTest >> testRemoveKeyWithTwoTransactions [ self should: [tx commit] raise: SoilObjectHasConcurrentChange ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testSecond [ - dict at: #foo put: #bar. - dict at: #foo2 put: #bar2. - - self assert: dict second equals: #bar2 -] - { #category : #tests } SoilIndexedDictionaryTest >> testSecondWithTransaction [ | tx tx2 | @@ -659,14 +518,6 @@ SoilIndexedDictionaryTest >> testSecondWithTransaction [ self assert: tx2 root second equals: #two ] -{ #category : #tests } -SoilIndexedDictionaryTest >> testSize [ - dict at: #foo put: #bar. - dict at: #foo2 put: #bar2. - - self assert: dict size equals: 2 -] - { #category : #tests } SoilIndexedDictionaryTest >> testSizeWithTransaction [ | tx tx2 | @@ -681,12 +532,3 @@ SoilIndexedDictionaryTest >> testSizeWithTransaction [ "and test last" self assert: tx2 root size equals: 2 ] - -{ #category : #tests } -SoilIndexedDictionaryTest >> testValues [ - dict at: #foo put: #bar. - dict at: #foo2 put: #bar2. - - self assert: (dict values includes: 'bar'). - self assert: (dict values includes: 'bar2') -] diff --git a/src/Soil-Core/SoilIndexedDictionary.class.st b/src/Soil-Core/SoilIndexedDictionary.class.st index 2dd0e269..397e41ce 100644 --- a/src/Soil-Core/SoilIndexedDictionary.class.st +++ b/src/Soil-Core/SoilIndexedDictionary.class.st @@ -34,44 +34,37 @@ SoilIndexedDictionary >> at: key [ { #category : #accessing } SoilIndexedDictionary >> at: key ifAbsent: aBlock [ | objectId | - ^ transaction - ifNotNil: [ - objectId := (self basicAt: key ifAbsent: [ ^ aBlock value ]) asSoilObjectId. - transaction proxyForObjectId: objectId ] - ifNil: [ newValues at: key ifAbsent: aBlock ] + + objectId := (self basicAt: key ifAbsent: [ ^ aBlock value ]) asSoilObjectId. + ^ transaction proxyForObjectId: objectId + ] { #category : #accessing } SoilIndexedDictionary >> at: key put: anObject [ - ^ transaction - ifNotNil: [ - | objectId iterator | - objectId := transaction makeRoot: anObject. - "binKey := (key asSkipListKeyOfSize: index keySize) asInteger." - iterator := self index newIterator. - (iterator at: key put: objectId) ifNotNil: [ :value | - oldValues - at: key - ifAbsentPut: objectId ]. - "if there has been a prior removal of the key this new - addition invalidates it" - removedValues removeKey: key ifAbsent: nil. - newValues at: key put: objectId. ] - ifNil: [ - newValues at: key put: anObject ] + + | objectId iterator | + objectId := transaction makeRoot: anObject. + "binKey := (key asSkipListKeyOfSize: index keySize) asInteger." + iterator := self index newIterator. + (iterator at: key put: objectId) ifNotNil: [ :value | + oldValues + at: key + ifAbsentPut: objectId ]. + "if there has been a prior removal of the key this new addition invalidates it" + removedValues removeKey: key ifAbsent: nil. + newValues at: key put: objectId ] { #category : #accessing } SoilIndexedDictionary >> atIndex: anInteger [ - ^ transaction - ifNotNil: [ - (self index atIndex: anInteger) - ifNotNil: [ :bytes | - transaction - objectId: bytes asSoilObjectId - ifVisible: [:objectId | (objectId asSoilObjectProxy) transaction: transaction ] - ifHidden: nil ] ] - ifNil: [ (newValues associations at: anInteger) value ] + + ^ (self index atIndex: anInteger) ifNotNil: [ :bytes | + transaction + objectId: bytes asSoilObjectId + ifVisible: [ :objectId | + objectId asSoilObjectProxy transaction: transaction ] + ifHidden: nil ] ] { #category : #accessing } @@ -96,46 +89,35 @@ SoilIndexedDictionary >> createIndex [ { #category : #enumerating } SoilIndexedDictionary >> do: aBlock [ - transaction - ifNotNil: [ - | iterator assoc | - iterator := self index newIterator. - [ (assoc := iterator nextAssociation) notNil ] whileTrue: [ - (self - restoreValue: assoc value - forKey: assoc key - iterator: iterator) ifNotNil: [ :objectId | - aBlock value: (transaction proxyForObjectId: objectId) ] ] ] - ifNil: [ - newValues valuesDo: [ :each | aBlock value: each ] ] + | iterator assoc | + iterator := self index newIterator. + [ (assoc := iterator nextAssociation) notNil ] whileTrue: [ + (self + restoreValue: assoc value + forKey: assoc key + iterator: iterator) ifNotNil: [ :objectId | + aBlock value: (transaction proxyForObjectId: objectId)]] ] { #category : #accessing } SoilIndexedDictionary >> first [ - ^ transaction - ifNotNil: [ self proxyFromByteArray: self index newIterator first ] - ifNil: [ - self newValuesSortedByKeyOrder ifNotEmpty: [:nv | nv first value ] ifEmpty: nil] + ^ self proxyFromByteArray: self index newIterator first + ] { #category : #accessing } SoilIndexedDictionary >> first: anInteger [ - ^ transaction - ifNotNil: [ - (self index first: anInteger) - collect: [ :each | self proxyFromByteArray: each ] ] - ifNil: [ (self newValuesSortedByKeyOrder first: anInteger) collect: #value ] + + ^ (self index first: anInteger) + collect: [ :each | self proxyFromByteArray: each ] ] { #category : #accessing } SoilIndexedDictionary >> firstAssociation [ - ^ transaction - ifNotNil: [ - index newIterator firstAssociation ifNotNil: [ :assoc | - assoc key -> (transaction objectWithId: assoc value asSoilObjectId) ] ] - ifNil: [ - self newValuesSortedByKeyOrder ifNotEmpty: [:nv | nv first ] ifEmpty: nil] + ^ index newIterator firstAssociation ifNotNil: [ :assoc | + assoc key -> (transaction objectWithId: assoc value asSoilObjectId) ] + ] { #category : #testing } @@ -192,13 +174,12 @@ SoilIndexedDictionary >> isEmpty [ | iterator| iterator := self index newIterator. iterator currentPage: self index firstPage. - ^ newValues isEmpty and: [ self index isEmpty - or: [ + ^ self index isEmpty or: [ "all items might be removed and not restorable" (self index firstPage items allSatisfy: [ :each | (self restoreValue: each value forKey: each key - iterator: iterator)isNil] )]] + iterator: iterator)isNil] )] ] { #category : #testing } @@ -213,21 +194,14 @@ SoilIndexedDictionary >> keySize: anInteger [ { #category : #accessing } SoilIndexedDictionary >> last [ - ^ transaction - ifNotNil: [ self proxyFromByteArray: self index newIterator last ] - ifNil: [ - self newValuesSortedByKeyOrder ifNotEmpty: [:nv | nv last value ] ifEmpty: nil ] + ^ self proxyFromByteArray: self index newIterator last ] { #category : #accessing } SoilIndexedDictionary >> lastAssociation [ - ^ transaction - ifNotNil: [ - index newIterator lastAssociation ifNotNil: [ :assoc | - assoc key -> (transaction objectWithId: assoc value asSoilObjectId) ] ] - ifNil: [ - self newValuesSortedByKeyOrder ifNotEmpty: [:nv | nv last ] ifEmpty: nil ] + ^ index newIterator lastAssociation ifNotNil: [ :assoc | + assoc key -> (transaction objectWithId: assoc value asSoilObjectId) ] ] { #category : #private } @@ -243,21 +217,9 @@ SoilIndexedDictionary >> maxLevel: anInteger [ ] -{ #category : #accessing } -SoilIndexedDictionary >> newValuesSortedByKeyOrder [ - - ^ newValues associations sort: [ :a :b | - (a key asSkipListKeyOfSize: self index keySize) asInteger - < (b key asSkipListKeyOfSize: self index keySize) asInteger ] -] - { #category : #accessing } SoilIndexedDictionary >> nextAfter: key [ | iterator | - transaction ifNil: [ - | newValueSorted | - newValueSorted := self newValuesSortedByKeyOrder. - ^ (newValueSorted after: (newValues associationAt: key)) value ]. iterator := self index newIterator find: key asInteger; @@ -267,13 +229,6 @@ SoilIndexedDictionary >> nextAfter: key [ assoc key -> (transaction objectWithId: assoc value asSoilObjectId) ] ] -{ #category : #private } -SoilIndexedDictionary >> prepareNewValues [ - newValues copy keysAndValuesDo: [ :key :object | - object isObjectId ifFalse: [ - newValues at: key put: (transaction makeRoot: object) ] ] -] - { #category : #printing } SoilIndexedDictionary >> printOn: aStream [ super printOn: aStream. @@ -296,21 +251,14 @@ SoilIndexedDictionary >> removeKey: key [ { #category : #removing } SoilIndexedDictionary >> removeKey: key ifAbsent: aBlock [ | iterator v | - ^ transaction - ifNotNil: [ - "remove from newValues as there could be a new at:put: on that - key but removing the key will remove the value again" - newValues removeKey: key ifAbsent: nil. - iterator := self index newIterator. - v := self basicAt: key ifAbsent: [^ aBlock value]. - removedValues - at: key - put: v asSoilObjectId. - iterator at: key put: (SoilObjectId segment: 0 index: 0) ] - ifNil: [ - removedValues - at: key - put: (newValues removeKey: key ifAbsent: [ ^ aBlock value ]) ] + "remove from newValues as there could be a new at:put: on that key but removing the key will remove the value again" + newValues removeKey: key ifAbsent: nil. + iterator := self index newIterator. + v := self basicAt: key ifAbsent: [^ aBlock value]. + removedValues + at: key + put: v asSoilObjectId. + ^ iterator at: key put: (SoilObjectId segment: 0 index: 0) ] { #category : #private } @@ -333,23 +281,18 @@ SoilIndexedDictionary >> restoreValue: value forKey: key iterator: iterator [ { #category : #accessing } SoilIndexedDictionary >> second [ - ^ transaction - ifNotNil: [ self proxyFromByteArray: (index newIterator first; next) ] - ifNil: [ self newValuesSortedByKeyOrder second value ] + ^ self proxyFromByteArray: (index newIterator first; next) ] { #category : #accessing } SoilIndexedDictionary >> size [ - ^ transaction - ifNotNil: [ self index size ] - ifNil: [ newValues size ] + ^ self index size ] { #category : #serializing } SoilIndexedDictionary >> soilBasicSerialize: aSerializer [ transaction ifNil: [ - transaction := aSerializer transaction. - self prepareNewValues ]. + transaction := aSerializer transaction]. super soilBasicSerialize: aSerializer. aSerializer registerIndexId: id. ] @@ -358,14 +301,6 @@ SoilIndexedDictionary >> soilBasicSerialize: aSerializer [ SoilIndexedDictionary >> soilClusterRootIn: aTransaction [ transaction ifNotNil: [ ^ self ]. transaction := aTransaction. - newValues copy keysAndValuesDo: [ :key :object | | obj | - obj := object isObjectId - ifTrue: [ object ] - ifFalse: [ - newValues - at: key - put: (transaction makeRoot: object) ]. - self index newIterator at: key put: obj ]. transaction markDirty: self ]