Skip to content

Conversation

@osa1
Copy link
Member

@osa1 osa1 commented Aug 25, 2022

Currently deepCopy and clone create an empty message, and then merge the
cloned message into the empty message.

This is slow as merge needs to validate each element added to the message.

In this PR we instead directly copy the field set, and deeply copy map, list,
and message fields.

deepCopy benchmark results:

Before After Diff
AOT 238 us 151 us -36%
dart2js -O4 242 us 140 us -42%
dart2wasm -O2 230 us 123 us -46%
JIT 189 us 104 us -44%

This CL is tested internally in cl/794080754.

Fixes #60.

@osa1
Copy link
Member Author

osa1 commented Sep 6, 2022

This causes a lot of breakage internally. I think one of the reasons is probably that I'm assuming PbList for repeated fields and PbMap for map fields, but map and list allocation is done via GeneratedMessage.createRepeatedField (returns List<T>) and GeneratedMessage.createMapField (returns Map<K, V>). I should check the field type (instead of field value type), and use Map and List API similar to how we merge map and list fields.

Since we can't have nested lists and maps, cloning of nested maps and lists will have to go through GeneratedMessage.deepCopy, which will again look at field types and use List/Map API to clone maps and lists. So we won't need deepCopy in PbMap and PbList types.

Interestingly, we assume PbList and PbMap in _FieldSet._markReadOnly:

void _markReadOnly() {
  if (_isReadOnly) return;
  _frozenState = true;
  for (var field in _meta.sortedByTag) {
    if (field.isRepeated) {
      PbList? list = _values[field.index!];
      if (list == null) continue;
      list.freeze();
    } else if (field.isMapField) {
      PbMap? map = _values[field.index!];
      if (map == null) continue;
      map.freeze();
    } else if (field.isGroupOrMessage) {
      final entry = _values[field.index!];
      if (entry != null) {
        (entry as GeneratedMessage).freeze();
      }
    }
  }

  _extensions?._markReadOnly();
  _unknownFields?._markReadOnly();
}

If I'm right that the breakage here caused by users overriding list and map field types, then I'm not sure how _markReadOnly works. Maybe users that override map and list types never freeze their messages?

We should just rewrite this library..

@osa1
Copy link
Member Author

osa1 commented Sep 8, 2022

I managed to fix at least some of the downstream breakage by removing the assumptions in the implementation about repeated fields being PbList and map fields being PbMap.

The fact that we csan't assume this means freezing messages is buggy, for the reasons described in my previous comment above.

Performance is still much better than merge-based implementation. I will update the PR description, and refactor the code a little bit (document, revert some of the changes).

@osa1
Copy link
Member Author

osa1 commented Sep 9, 2022

I managed to fix at least some of the downstream breakage by removing the assumptions in the implementation about repeated fields being PbList and map fields being PbMap.

This version fixed some of the previous failures, but introduced new ones.

@osa1
Copy link
Member Author

osa1 commented Jul 28, 2025

We should revive this as it improves cloning performance quite a bit (50% according to my original report in PR description).

As far as I understand from the discussion above the main issue was repeated and map fields were overridden by message classes, but we've since removed those members (96d9522) so this should probably be good now.

It might make sense to just implement this again from scratch instead of fixing this PR.

@osa1 osa1 marked this pull request as draft August 14, 2025 07:27
@github-actions
Copy link

github-actions bot commented Aug 14, 2025

Package publishing

Package Version Status Publish tag (post-merge)
package:protobuf 5.0.0 (error) pubspec version (5.0.0) and changelog (5.0.0-wip) don't agree
package:protoc_plugin 23.0.0 ready to publish protoc_plugin-v23.0.0

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@osa1 osa1 marked this pull request as ready for review August 14, 2025 08:06
@osa1 osa1 requested a review from devoncarew August 14, 2025 08:06
@osa1
Copy link
Member Author

osa1 commented Aug 14, 2025

@devoncarew @sigurdm I rewrote this PR from scratch and force-pushed (it was bitrot). I resolved all previous discussions because the code is different.

Note: the plugin changes are not synced to the CL yet and not tested internally yet. I'll sync it later today and start a global presubmit with the plugin changes. The library changes are tested globally. Update: it's being tested now.

@osa1 osa1 requested a review from sigurdm August 14, 2025 09:34
Copy link
Collaborator

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -1,5 +1,9 @@
## 5.0.0-wip

* Improve performance of `GeneratedMessage.deepCopy`. ([#742])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the pubspec version to agree w/ the changelog (=> 5.0.0-wip). If we do plan to publish after landing this (no particular opinion), we can change both versions to 5.0.0).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we discuss this already? #1037 (comment)

5.0.0-wip breaks the build because it's not considered 5.0.0, and plugin now needs 5.0.0.

We can update the changelog before or even after releasing 5.0.0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I'd forgotten.

@osa1 osa1 merged commit 19e5a25 into google:master Aug 15, 2025
12 checks passed
titanous pushed a commit to titanous/protobuf.dart that referenced this pull request Aug 15, 2025
Currently `deepCopy` and `clone` create an empty message, and then merge the
cloned message into the empty message.

This is slow as merge needs to validate each element added to the message.

In this PR we instead directly copy the field set, and deeply copy map, list,
and message fields.

deepCopy benchmark results:

|                  | Before  | After   | Diff   |
|------------------|---------|---------|--------|
| AOT              | 238 us  | 151 us  | -36%   |
| dart2js -O4      | 242 us  | 140 us  | -42%   |
| dart2wasm -O2    | 230 us  | 123 us  | -46%   |
| JIT              | 189 us  | 104 us  | -44%   |

This CL is tested internally in cl/794080754.

Fixes google#60.
@osa1 osa1 deleted the deepCopy branch August 15, 2025 18:16
Spiritus2424 pushed a commit to EXFO/protobuf.dart that referenced this pull request Aug 28, 2025
Currently `deepCopy` and `clone` create an empty message, and then merge the
cloned message into the empty message.

This is slow as merge needs to validate each element added to the message.

In this PR we instead directly copy the field set, and deeply copy map, list,
and message fields.

deepCopy benchmark results:

|                  | Before  | After   | Diff   |
|------------------|---------|---------|--------|
| AOT              | 238 us  | 151 us  | -36%   |
| dart2js -O4      | 242 us  | 140 us  | -42%   |
| dart2wasm -O2    | 230 us  | 123 us  | -46%   |
| JIT              | 189 us  | 104 us  | -44%   |

This CL is tested internally in cl/794080754.

Fixes google#60.
Spiritus2424 pushed a commit to EXFO/protobuf.dart that referenced this pull request Aug 28, 2025
Currently `deepCopy` and `clone` create an empty message, and then merge the
cloned message into the empty message.

This is slow as merge needs to validate each element added to the message.

In this PR we instead directly copy the field set, and deeply copy map, list,
and message fields.

deepCopy benchmark results:

|                  | Before  | After   | Diff   |
|------------------|---------|---------|--------|
| AOT              | 238 us  | 151 us  | -36%   |
| dart2js -O4      | 242 us  | 140 us  | -42%   |
| dart2wasm -O2    | 230 us  | 123 us  | -46%   |
| JIT              | 189 us  | 104 us  | -44%   |

This CL is tested internally in cl/794080754.

Fixes google#60.
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 28, 2025
Revisions updated by `dart tools/rev_sdk_deps.dart`.

ecosystem (https://github.com/dart-lang/ecosystem/compare/68ff911..fd28be2):
  fd28be2  2025-08-27  Moritz  Dont show warnings by default (dart-lang/ecosystem#365)

http (https://github.com/dart-lang/http/compare/6656f15..ef05b37):
  ef05b37  2025-08-27  Brian Quinlan  [cupertino_http]: Switch to ffigen 19.1.0 (dart-lang/http#1811)

i18n (https://github.com/dart-lang/i18n/compare/6a8f9c7..9a211d1):
  9a211d14  2025-08-27  Moritz  [package:intl4x] `DateTime` formatting redesign (dart-lang/i18n#1003)

protobuf (https://github.com/dart-lang/protobuf/compare/6e9c9f4..765ba8a):
  765ba8a  2025-08-28  Ömer Sinan Ağacan  Fix non-proto3 JSON format double decoding (google/protobuf.dart#1043)
  703bf7c  2025-08-27  Ömer Sinan Ağacan  Compile dart2wasm benchmarks with --no-strip-wasm (google/protobuf.dart#1044)
  9939965  2025-08-26  Ömer Sinan Ağacan  Fix JS interop annotations (google/protobuf.dart#1042)
  da354a2  2025-08-22  Ömer Sinan Ağacan  Update benchmark builder to use -O2 with dart2wasm, add output dir to gitignore (google/protobuf.dart#1041)
  41eba19  2025-08-22  Ömer Sinan Ağacan  Add sinks to benchmarks to prevent smart tools eliminating benchmarked code (google/protobuf.dart#1040)
  3ccc8ab  2025-08-19  dependabot[bot]  Bump actions/checkout from 3.5.3 to 4.2.2 (google/protobuf.dart#1036)
  6bce9d7  2025-08-19  Ömer Sinan Ağacan  Fix unknown enum handling (google/protobuf.dart#853)
  19e5a25  2025-08-15  Ömer Sinan Ağacan  Implement deepCopy as copying, instead of merge (google/protobuf.dart#742)

web (https://github.com/dart-lang/web/compare/ee9733f..e7895bd):
  e7895bd  2025-08-27  Nikechukwu  [interop] Add Support for Declaration Merging and Overloading (dart-lang/web#449)

webdev (https://github.com/dart-lang/webdev/compare/9724fea..a7d3d2f):
  a7d3d2fc  2025-08-27  Ben Konyi  [ DWDS ] Fix issue where null was repeatedly sent to connected clients (dart-lang/webdev#2678)
  e3009dfb  2025-08-27  Nate Biggs  Remove stale calls to dartSdkIsAtLeast. (dart-lang/webdev#2677)
  e0d58082  2025-08-26  Nate Biggs  Fix expectation in test accessing private field. (dart-lang/webdev#2676)

Change-Id: Ia98a8f065830865f6922ca032939380683645d72
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/447640
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
Auto-Submit: Devon Carew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cloning should avoid validation checks

3 participants