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

Define the resources.json format from @ResourceIdentifiers and @pragma('dart2js:resource-identifier') #55494

Open
Tracked by #55555
dcharkes opened this issue Apr 17, 2024 · 5 comments
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...).

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Apr 17, 2024

Last year the concept of resource identifiers was introduced.

Currently, there are some inconsistencies between the format output by the VM and by dart2js.

  • dart2js does not output an id (The ResourceIdentifier.metadata field in the package:meta annotation.)
  • nonconstant vs nonConstant spelling. (Shouldn't a bool just be constant? Why use the negation?)

The current serialization is centered around loading units, but not all use cases might require loading units.

The current serialization contains source locations and loading units unconditionally, but this is fragile if the format is used for caching. So we should consider making these optional, depending on use case.

The format should be versioned.

A sketch of what the serialization could look like when addressing these issues:

{
  "_comment": "Resources referenced by annotated resource identifers",
  "_version": [
    1,
    0,
    0
  ],
  "identifiers": [
    {
      "uri": "org-dartlang-sdk:///lib/_internal/js_runtime/lib/js_helper.dart",
      "containerType": "class",
      "container": "MyClass",
      "name": "loadDeferredLibrary",
      "constant": true,
      "references": [
        {
          "arguments": {
            "1": "lib_SHA1",
            "2": 0
          },
          "loadingUnit": "o.js",
          "@": {
            "uri": "file://benchmarks/OmnibusDeferred/dart/OmnibusDeferred.dart",
            "line": 14,
            "column": 48
          }
        }
      ]
    }
  ]
}

@rakudrama I'd like to understand what the requirements are from dart2js. Then we can figure out if the requirements are compatible with the on for link hooks, so that we can settle on a single format. (Hopefully yes, if we make some things optional. I believe dart2js explicitly wants to use loading units for example.)

I have done a long write-up of the requirements for the resources.json in link hooks here:

cc @mosuem @mkustermann @natebosch Feel free to chime in!
cc @johnniwinther Having a concept of fully qualified (unique) names would help us to avoid uri-containerType-container-name, but I believe we have no such thing in Dart?

@johnniwinther
Copy link
Member

We don't have fully qualified name but be somewhat created using the import uri (aka canonical uri) of the library and the qualified name from within the library (taking getter/setter into account) similar to how we create canonical names for serialization.

@mraleph mraleph added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Apr 18, 2024
@dcharkes
Copy link
Contributor Author

Thanks @rakudrama for the good chat!

Some notes:

  • We have one user of this format via dart2js, Including the loading units: ACX. The format was implemented to serve that use case and wasn't implemented further. We can do breaking changes to the format as long as we also migrate this use.
  • The source URI/line/column is to be able to give useful error messages when consuming the file. E.g. from hook/link.dart in our new use case.
    • For caching, we could consider using a projection from the format that hides the source information to avoid running link hooks again.
  • notconstant is an optimization so that not all references have to be read.
  • Adding a version to the format is a good idea.
  • Adding container (class/extension) and container type would be non breaking changes.
  • So would adding support for get:xxx and set:xxx (where getters would have no parameters, so they could only be used for saying a resource is used at all yes or no).

@mosuem
Copy link
Member

mosuem commented May 22, 2024

After a discussion offline with @dcharkes, we came up with:

A recordedReference consists of

  • the annotation(s) causing the reference to be recorded, together with their fields, to make them extensible Make it possible to implement ResourceIdentifier #55568.
  • the definitions, uniquely identified by URI, parent, and name.
    • The name can be get:xxx or set:xxx.
  • the references with their arguments.
  • The arguments have explicit positional and named arguments, and explicit const and non const argument values.
  • Now that optional non-passed parameters are distinguishable from non const parameters, a nonconstant field is no longer needed.
  • The loading units are a property of definitions and references now.
  • The definitions now also have source positions.
  • The hashes can be used for more efficient caching. (Source positions and loading units might not warrant reruns for some use cases.)
{
    "metadata": {
        "comment": "Recorded references at compile time and their argument values, as far as known, to definitions annotated with @RecordReference",
        "version": "1.6.2-wip+5.-.2.z",
        "timestamp": 321432153,
        "hashes": {
            "noPositions": "dasdsadfasfwagwraf",
            "noPositionsNoLoadingUnits": "fdsfdsafdsagh"
        }
    },
    "staticFunctionReferences": [
        {
            "annotations": [
                {
                    "identifier": {
                        "uri": "file://lib/_internal/js_runtime/lib/js_helper.dart",
                        "name": "RecordReference"
                    },
                    "fields": {
                        "_metadata": {
                            "key": true
                        }
                    }
                }
            ],
            "definition": {
                "identifier": {
                    "uri": "file://lib/_internal/js_runtime/lib/js_helper.dart",
                    "parent": "MyClass",
                    "name": "get:loadDeferredLibrary"
                },
                "@": {
                    "uri": "file://lib/_internal/js_runtime/lib/js_helper.dart",
                    "line": 12,
                    "column": 67
                },
                "loadingUnit": "part_15.js"
            },
            "references": [
                {
                    "arguments": {
                        "const": {
                            "positional": {
                                "0": "lib_SHA1",
                                "1": false,
                                "2": 1
                            },
                            "named": {
                                "leroy": "jenkins",
                                "freddy": "mercury"
                            }
                        },
                        "nonConst": {
                            "positional": [],
                            "named": []
                        }
                    },
                    "loadingUnit": "o.js",
                    "@": {
                        "uri": "file://benchmarks/OmnibusDeferred/dart/OmnibusDeferred.dart",
                        "line": 14,
                        "column": 48
                    }
                },
                {
                    "arguments": {
                        "const": {
                            "positional": {
                                "0": "lib_SHA1",
                                "2": 0
                            },
                            "named": {
                                "leroy": "jenkins"
                            }
                        },
                        "nonConst": {
                            "positional": [
                                "1"
                            ],
                            "named": [
                                "freddy"
                            ]
                        }
                    },
                    "loadingUnit": "o.js",
                    "@": {
                        "uri": "file://benchmarks/OmnibusDeferred/dart/OmnibusDeferred.dart",
                        "line": 14,
                        "column": 48
                    }
                }
            ]
        }
    ]
}

Any thoughts or input @rakudrama?

auto-submit bot pushed a commit to flutter/flutter that referenced this issue May 22, 2024
This PR adds support invoking `link.dart` hooks.

Link hooks can add new assets. Link hooks can transform assets sent to link hook from build hooks.

This PR does not yet add support for getting tree-shake information in the link hooks. This is pending on defining the `resources.json` format (dart-lang/sdk#55494).

Issue:

* #146263

## Implementation considerations

The build hooks could be run before Dart compilation and the link hooks after Dart compilation. (This is how it's done in Dart standalone.) However, due to the way the `Target`s are set up, this would require two targets and serializing and deserializing the `BuildResult` in between these. This would lead to more code but no benefits. Currently there is nothing that mandates running build hooks before Dart compilation.

## Testing

* The unit tests verify that the native_assets_builder `link` and `linkDryRun` would be invoked with help of the existing fake.
* The native assets integration test now also invokes an FFI call of a package that adds the asset during the link hook instead of the build hook.
  * In order to keep coverage of the `flutter create --template=package_ffi`, `flutter create` is still run and the extra dependency is added and an extra ffi call is added. (Open to alternative suggestions.)
@dcharkes
Copy link
Contributor Author

@mkustermann Asked if there is something we could do to make this format smaller.

  • We could for example add a URI-map to avoid repeating the same URI multiple times.
  • We could add an identifier-map to avoid repeating the same URI+identifier string multiple times.

The question is whether we should optimize for size. This file will only live on host machines, it's not part of a compiled app bundle. Also, since the contents of this format are filtered on the annotations, maybe they won't grow so big.

(If we decide we want to tackle size later, we'd have to go with uri_v2 a la dart-lang/native#93 (comment), and afterwards remove uri.)

@mkustermann You mentioned some other AOT treeshaking format in the SDK that is size optimized. Do you have a reference to it?

@dcharkes
Copy link
Contributor Author

RE: hashes:

We are looking into more sophisticated caching schemes for depending on part of the resources.json (dart-lang/native#1178). So it could make sense to either

  1. remove the hashes and have the consumer (native_assets_builder) rehash for what it wants to do caching on, or
  2. have hashes per annotation class (which is what the native_assets_builder would use most likely).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...).
Projects
None yet
Development

No branches or pull requests

4 participants