Add ReadOnly iterators and refactor other iterators #329
Conversation
This change: - Adds ReadOnly iterators that match current iterator API (except for the "ReadOnly" suffix added to some function names). - Refactors API of non-Readonly iterators because register inlining will require more parameters for MapIterator. For ReadOnly iterators, the caller is responsible for preventing changes to child containers during iteration because mutations of child containers are not guaranteed to persist. For non-ReadOnly iterators, two additional parameters are needed to update child container in parent map when child container is modified.
fxamacker
added
the
breaking change
| @@ -65,7 +65,7 @@ func verifyArray( | |||
|
|
|||
| // Verify array elements by iterator | |||
| i := 0 | |||
| err = array.Iterate(func(v Value) (bool, error) { | |||
| err = array.IterateReadOnly(func(v Value) (bool, error) { | |||
There was a problem hiding this comment.
Are there any tests that still utilize the old APIs, like Iterate, IterateRange, etc.?
There was a problem hiding this comment.
Basically, the old is the same as new API implementation until register inlining is added. So (for now) the tests for the new APIs currently cover the old functionality.
The old APIs like Iterate and IterateRange, etc. currently have an incomplete implementation regarding register inlining, so the tests for them will be done after they are implemented.
| // ReadOnlyIterator returns readonly iterator for array elements. | ||
| // If elements of child containers are mutated, those changes | ||
| // are not guaranteed to persist. | ||
| func (a *Array) ReadOnlyIterator() (*ArrayIterator, error) { |
There was a problem hiding this comment.
More of a question on use-cases: What is the advantage of using the read-only iterators over the non-read-only ones? Is there a performance gain?
There was a problem hiding this comment.
Good questions. From performance & API perspectives:
-
performance: non-readonly iterator needs to keep track of child mutable elements so when child elements are modified, parent container would re-inline them. So there is overhead and additional data for tracking, etc.
-
API: array iterators (readonly and non-readonly) would remain the same. However, non-readonly map iterators need additional parameters to generate hash for map key and compare map key. I think there are enough use cases for client code iterating elements without changes, so requiring these extra parameters is overkill.
Most likely, I'll postpone merging this PR until after register inlining is implemented (in a separate PR). So we'll have a chance to tweak the API if needed before this PR is merged.
There was a problem hiding this comment.
As discussed in the review session today, this looks good, and while there are pretty much no cases where these functions are useful for Cadence programs, they are useful for many cases where Cadence itself iterates and does not mutate.
I assume the PR is not actually a breaking change and only adds the new Read... functions?
Ideally the PR should not change the behavior of the existing iteration functions and keep assuming the callback might mutate. This way Cadence keeps working and can start to adopt the read-only iterators where it makes sense and is safe.
|
@fxamacker Could you please open a tech-debt follow-up issue in the Cadence repo, so that we don't forget to switch Cadence to the new read-only versions where possible? |
|
I'm replacing this PR with a new one that supports mutation of values from non-readonly iterators. The replacement PR will also include the atree inlining changes from PR #342. A followup issue will also be opened in onflow/cadence repo. |
|
This PR is replaced by PR #345 |
Updates #292
Changes:
For ReadOnly iterators, the caller is responsible for preventing changes to child containers during iteration because mutations of child containers are not guaranteed to persist.
For non-ReadOnly iterators, two additional parameters are needed to update child container in parent map when child container is modified.
mainbranchFiles changedin the Github PR explorer