Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider the special case for caching read-only repeated field values #706

Open
osa1 opened this issue Jul 11, 2022 · 0 comments
Open
Labels
question refactoring Non-user-facing changes

Comments

@osa1
Copy link
Member

osa1 commented Jul 11, 2022

dynamic get readonlyDefault {
if (isRepeated) {
return _emptyList ??= PbList.unmodifiable();
}
return makeDefault!();
}

/// Cached read-only empty list for this field type. For non-repeated fields
/// this is always `null`. Otherwise it starts as `null` and gets initialized
/// in `readonlyDefault`.
PbList<T>? _emptyList;

I don't understand why special-case lists here. Why not also cache maps and messages? (i.e. other allocated types)

Since this field exists in all fields, it's one word overhead for non-repeated fields.


We find bug after bug after bug in this library because because of the special cases around types in various code. Examples:

IMO a major code quality issue in this library is all these special cases about field types.

Part of the reason for these special cases is we can't introduce a class hierarchy for protobuf values as that would be a layer of indirection for "simple" field types like int32, bool, etc. which we currently represent as Dart int, bool, and so on. Performance impact of having a class PbInt32 implements PbValue would probably be unacceptable.

I think we should avoid these special cases as much as possible.

@osa1 osa1 added refactoring Non-user-facing changes question labels Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question refactoring Non-user-facing changes
Projects
None yet
Development

No branches or pull requests

1 participant