Dart null safety #6696
Dart null safety #6696
Conversation
|
Generally looks good to me, but would like @dnfield to review. |
| } | ||
|
|
||
| BufferContext._(this._buffer); | ||
| BufferContext(this._buffer); |
There was a problem hiding this comment.
Why are we making this public?
If we need it to be public we should document it.
There was a problem hiding this comment.
Hmm, that slipped in while I've been comparing/merging with the null-safe version from objectbox - it's used there for performance reasons - no need for list of bytes to construct if ByteData is the actually needed interface.
So while this is unrelated to the current PR, I'd leave it here as it doesn't hurt anyone and actually provides a bit of value for lib users. Added a doc comment.
There was a problem hiding this comment.
SG, docs haven't shown up here yet though (forgot to save/push?)
There was a problem hiding this comment.
it was there and pushed, not sure why it doesn't show up 304056a
| @@ -126,15 +129,15 @@ class Builder { | |||
|
|
|||
| /// The location of the end of the current table, measured in bytes from the | |||
| /// end of [_buf], or `null` if a table is not currently being built. | |||
| int _currentTableEndTail; | |||
| int _currentTableEndTail = 0; | |||
There was a problem hiding this comment.
Is zero the right sentinel value to use?
Doc comment needs to be updated, since it can't be null anymore.
There was a problem hiding this comment.
it is never accessed before writing, setting a value just makes it a bit faster internally because dart doesn't have to consider it ever being null. Alternatively, I can change it to late though I guess that comes with some runtime overhead.
| @@ -111,7 +113,8 @@ class Builder { | |||
|
|
|||
| /// The list of existing VTable(s). | |||
| //final List<_VTable> _vTables = <_VTable>[]; | |||
| final List<int> _vTables = <int>[]; | |||
| final List<int> _vTables = List<int>.filled(16, 0, growable: true) | |||
There was a problem hiding this comment.
If you don't mind, can you remove the commented out line above this one while you're here?
Also - why are we changing this here? Are there benchmarks that showing a starting size of 16 is better than whatever the core SDK decides to start with?
There was a problem hiding this comment.
Sure, removed the commented out code.
Similarly to #6696 (comment) - don't have plain benchmarks with and without this change at the moment, only measured as a whole as part of the larger change suite suggested by mraleph a few months back.
| @@ -444,7 +448,7 @@ class Builder { | |||
| _maxAlign = 1; | |||
| _tail = 0; | |||
| _currentVTable = null; | |||
| _vTables.clear(); | |||
| _vTables.length = 0; | |||
There was a problem hiding this comment.
I think this change should be made separately. Clear is the same as this.
There was a problem hiding this comment.
Another instance of a change that slipped in from the fork. Coming from some talks with @mraleph (this commit: objectbox/objectbox-dart@a729705#diff-594a84a4fd72af27284e6f59d689b04f9f1f8cfe9dbcdfb512669d16d0f702b0) - I was also a bit confused why setting length was chosen instead of clear but didn't ask at the time. Maybe this mention will prompt a helpful comment from @mraleph? Either way, I'm fine with rolling back to clear() if you want.
There was a problem hiding this comment.
Maybe it avoids a method invocation? Clear does this internally. Not really a big deal either way, but it might be good to separate out some of these performance related fixes into a separate patch. Not a big deal either way though, this shouldn't hurt anything.
There was a problem hiding this comment.
I don't actually remember why I made this change. Most likely clear() was not inlined, can't really find any explanation why I would do it otherwise.
There was a problem hiding this comment.
Would keep it this way if you don't mind. I can create a follow-up PR with the remaining perf improvments
closes #6282