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

Add package:record_use #1

Closed
wants to merge 23 commits into from
Closed

Add package:record_use #1

wants to merge 23 commits into from

Conversation

mosuem
Copy link
Member

@mosuem mosuem commented Jun 12, 2024

Add package:record_use. This is part of an effort to allow tree-shaking of non-Dart objects, such as data files or native code.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@mosuem mosuem marked this pull request as ready for review June 19, 2024 08:02
Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

My main comment here is around the use of protos. Introducing a transitive dep on that package to consumers is a large consideration. Additionally, the format is not typically used outside of Google specific projects (grpc, ...).

Are we expecting the files to be very large? The format to be difficult to encode / decode? Is evolving the format in backwards compatible ways important?

How would the tradeoffs of encoding to and from json compare?

.github/workflows/record_use.yaml Outdated Show resolved Hide resolved
pkgs/record_use/README.md Show resolved Hide resolved
To install the record_use package, run the following command:

```bash
dart pub add record_use
Copy link
Member

Choose a reason for hiding this comment

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

Typically (always?) this package will be added as a regular dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was copied from a generic package readme - I think the only package using this will be native_assets_cli...

import 'dart:convert';
import 'dart:typed_data';

import '../proto/usages_read.pb.dart' as pb;
Copy link
Member

Choose a reason for hiding this comment

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

Having all consumers of this package transitively take a dep on package:protobuf is a huge consideration. protobuf here is used to facilitate writing and reading usage information to disk? It that file format intended to be canonical (can the annotation be used meaningfully without understanding this file format)?

Copy link
Member

Choose a reason for hiding this comment

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

We haven't decided on the format yet, it may change before we put it out there.

Even if we decided for protobuf, we could have a copy of package:protobuf runtime code in lib/src (and regularly update it from upstream) to avoid adding more transitive deps

@mosuem
Copy link
Member Author

mosuem commented Jun 19, 2024

My main comment here is around the use of protos. Introducing a transitive dep on that package to consumers is a large consideration. Additionally, the format is not typically used outside of Google specific projects (grpc, ...).

This package will be a de-facto dev dependency, as it is only being used in link hooks. So the size should not be an issue, right?

Are we expecting the files to be very large? The format to be difficult to encode / decode? Is evolving the format in backwards compatible ways important?

Evolving the format and size are the two main drivers to go for protobuf, together with the ability to easily understand the format. The size could be large, depending on how user code looks like in practice, but the file would be only used at build time, so has no implications on released app size.

How would the tradeoffs of encoding to and from json compare?

JSON has a more difficult migration path for adding/removing support for keys, and is slightly larger in storage. It is easier to manually inspect stored files, which is why we would use the toDebugString() method to output debug files for verbose compilation modes. Protobuf has a schema which is simpler than JSON, and has better support for types.

I am fine with using JSON, but do like the easier migration with protobuf for future changes.

mosuem and others added 6 commits June 20, 2024 09:35
Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

lgtm and no more comments re: the file format. If we do find that this package is overly constraining the ecosystem re: the protobuf dep. / leaking the dep transitively to more places than expected we should be open to reexamining this choice.

// Generated code. Do not modify.
// source: usages_read.proto
//
// @dart = 2.12
Copy link
Member

@devoncarew devoncarew Jun 24, 2024

Choose a reason for hiding this comment

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

Note to self: 2.12 is now considered an old version of Dart; we should update the proto generator to use at least 3.0.

pkgs/record_use/README.md Show resolved Hide resolved

import 'package:record_use/record_use.dart';

void doStuff(RecordedUsages usage, Identifier callId, Identifier referenceId) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to provide an example of using RecordedUsages.fromFile? And / or borrow the main example from the readme.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think RecordedUsages.fromFile is an internal method/implementation detail, the users will get a deserialized RecordUsages object in their link.dart.

Copy link
Member

@mkustermann mkustermann left a comment

Choose a reason for hiding this comment

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

I think this will need some iteration, but tto get it started: lgtm with comments

Though I think it makes more sense for this package to live together with dart-lang/native: The users of this package will be hook/link.dart scripts which already depend on other packages in dart-lang/native.

@mosuem Could we land it there?

pkgs/record_use/README.md Show resolved Hide resolved
pkgs/record_use/README.md Show resolved Hide resolved
import 'dart:convert';
import 'dart:typed_data';

import '../proto/usages_read.pb.dart' as pb;
Copy link
Member

Choose a reason for hiding this comment

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

We haven't decided on the format yet, it may change before we put it out there.

Even if we decided for protobuf, we could have a copy of package:protobuf runtime code in lib/src (and regularly update it from upstream) to avoid adding more transitive deps

message Reference {
Location location = 1;
string loading_unit = 2;
oneof reference {
Copy link
Member

Choose a reason for hiding this comment

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

This is mixing two seperate and orthogonal concepts (just like the one RecordUse mixes different concepts into one).

I'd structure this more like

message Usages {
  repeated string uri_table = ...;
  repeated Constant constant_table = ...; // <-- A DAG of constants (similar to how kernel dumps its constants)
  repeated Member member_table = ...; // <-- Same terminology as in kernel
  repeated FieldReference field_reference_table;

  // This is information we recorded.
  repeated CallSite recorded_call_sites = ...;
  repeated ConstantUse recorded_constant_use = ...; // Decide whether to distinguish from below or not
  repeated ConstantAnnotationUse recorded_constant_annotation_use = ...;
}

// A recorded call site of a member
message CallSite {
  Location location = ...;
  Arguments arguments;
}

message Arguments {
  repeated ArgumentValue positionals;
  repeated NamedArgumentValue named;
}

message ArgumentValue {
  oneof value {
    bool non_constant = ...;
    int constant_index;    // <-- index into `Constant` array
  }
}

message Constant {
  oneof ... {
    int_constant = ...,
    double_constant = ....,
    instance_constant = ...,
    array_constant = ...,
    map_constant = ....
  }
}
message InstanceConstant {
  repeated int field_reference_index; //  index into a Usages.field_reference_table
  repeated int value_index; // index into Usages.constant_table
}
...

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that the usages have much in common:

  • A definition the annotation is placed on (a class or a method)
  • Additional information which is also recorded (arguments or fields)

The annotation also denotes a central concept: Record usages of the object in the code.

While there are also some differences, I do like this common "interface".

pkgs/record_use/pubspec.yaml Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants