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.

Sign up for GitHub

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

use LazyMap in extension constructors #45

Closed
wants to merge 1 commit into from
Closed

use LazyMap in extension constructors #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkgs/dart_model/lib/dart_model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@

export 'src/dart_model.dart';
export 'src/json.dart';
export 'src/json_buffer.dart' show LazyMap;
export 'src/json_changes.dart';
123 changes: 88 additions & 35 deletions pkgs/dart_model/lib/src/dart_model.g.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
// This file is generated. To make changes edit schemas/*.schema.json
// then run from the repo root: dart tool/dart_model_generator/bin/main.dart

import 'json_buffer.dart' show LazyMap;

/// An augmentation to Dart code. TODO(davidmorgan): this is a placeholder.
extension type Augmentation.fromJson(Map<String, Object?> node) {
Augmentation({
String? code,
}) : this.fromJson({
if (code != null) 'code': code,
});
}) : this.fromJson(LazyMap(
[
Copy link
Contributor Author

@jakemac53 jakemac53 Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fwiw, I don't love that we will allocate a list still for each call to these. It might be worth trying to some up with something better?

Copy link
Member

@rakudrama rakudrama Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The code also allocates a closure over this.
The per-object closure could be fixed by making this and explicit argument to the closure, and using a static method.

Calling the closure for each present key is likely to be O(N^2) as the switch-on-string generally compiles to an if-then-else chain on the string comparisons. For small N, no problem, but at some point all the string comparisons will dominate,

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I was thinking along these lines yesterday, it was great to find an implementation in my inbox this morning, I spent today adding some benchmarking on top and exploring a third way, writeup here

#48

tl;dr this is already nearly 3x faster than using standard maps; I think we can get rid of the list+closure using extension type builders, and it does give some further speedup, to 3.5x; still some way from proving that it works though, input and suggestions welcome :)

Copy link
Contributor Author

@jakemac53 jakemac53 Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The code also allocates a closure over this.

Is that still true given this is a redirecting constructor on an extension type? There is no this in scope.

Calling the closure for each present key is likely to be O(N^2) as the switch-on-string generally compiles to an if-then-else chain on the string comparisons. For small N, no problem, but at some point all the string comparisons will dominate,

For sure, but the number of keys is never going to be very large here, so I think it should be fine.

Thanks! I was thinking along these lines yesterday, it was great to find an implementation in my inbox this morning, I spent today adding some benchmarking on top and exploring a third way, writeup here

Cool, will take a look :)

if (code != null) 'code',
],
(key) => switch (key) {
'code' => code,
_ => null,
}));

/// Augmentation code.
String get code => node['code'] as String;
Expand All @@ -17,9 +24,14 @@ extension type Augmentation.fromJson(Map<String, Object?> node) {
extension type MetadataAnnotation.fromJson(Map<String, Object?> node) {
MetadataAnnotation({
QualifiedName? type,
}) : this.fromJson({
if (type != null) 'type': type,
});
}) : this.fromJson(LazyMap(
[
if (type != null) 'type',
],
(key) => switch (key) {
'type' => type,
_ => null,
}));

/// The type of the annotation.
QualifiedName get type => node['type'] as QualifiedName;
Expand All @@ -31,12 +43,18 @@ extension type Interface.fromJson(Map<String, Object?> node) {
List<MetadataAnnotation>? metadataAnnotations,
Map<String, Member>? members,
Properties? properties,
}) : this.fromJson({
if (metadataAnnotations != null)
'metadataAnnotations': metadataAnnotations,
if (members != null) 'members': members,
if (properties != null) 'properties': properties,
});
}) : this.fromJson(LazyMap(
[
if (metadataAnnotations != null) 'metadataAnnotations',
if (members != null) 'members',
if (properties != null) 'properties',
],
(key) => switch (key) {
'metadataAnnotations' => metadataAnnotations,
'members' => members,
'properties' => properties,
_ => null,
}));

/// The metadata annotations attached to this iterface.
List<MetadataAnnotation> get metadataAnnotations =>
Expand All @@ -53,9 +71,14 @@ extension type Interface.fromJson(Map<String, Object?> node) {
extension type Library.fromJson(Map<String, Object?> node) {
Library({
Map<String, Interface>? scopes,
}) : this.fromJson({
if (scopes != null) 'scopes': scopes,
});
}) : this.fromJson(LazyMap(
[
if (scopes != null) 'scopes',
],
(key) => switch (key) {
'scopes' => scopes,
_ => null,
}));

/// Scopes by name.
Map<String, Interface> get scopes => (node['scopes'] as Map).cast();
Expand All @@ -65,9 +88,14 @@ extension type Library.fromJson(Map<String, Object?> node) {
extension type Member.fromJson(Map<String, Object?> node) {
Member({
Properties? properties,
}) : this.fromJson({
if (properties != null) 'properties': properties,
});
}) : this.fromJson(LazyMap(
[
if (properties != null) 'properties',
],
(key) => switch (key) {
'properties' => properties,
_ => null,
}));

/// The properties of this member.
Properties get properties => node['properties'] as Properties;
Expand All @@ -77,9 +105,14 @@ extension type Member.fromJson(Map<String, Object?> node) {
extension type Model.fromJson(Map<String, Object?> node) {
Model({
Map<String, Library>? uris,
}) : this.fromJson({
if (uris != null) 'uris': uris,
});
}) : this.fromJson(LazyMap(
[
if (uris != null) 'uris',
],
(key) => switch (key) {
'uris' => uris,
_ => null,
}));

/// Libraries by URI.
Map<String, Library> get uris => (node['uris'] as Map).cast();
Expand All @@ -94,9 +127,14 @@ extension type NeverType.fromJson(Null _) {
extension type NullableType.fromJson(Map<String, Object?> node) {
NullableType({
StaticType? inner,
}) : this.fromJson({
if (inner != null) 'inner': inner,
});
}) : this.fromJson(LazyMap(
[
if (inner != null) 'inner',
],
(key) => switch (key) {
'inner' => inner,
_ => null,
}));
StaticType get inner => node['inner'] as StaticType;
}

Expand All @@ -109,14 +147,24 @@ extension type Properties.fromJson(Map<String, Object?> node) {
bool? isField,
bool? isMethod,
bool? isStatic,
}) : this.fromJson({
if (isAbstract != null) 'isAbstract': isAbstract,
if (isClass != null) 'isClass': isClass,
if (isGetter != null) 'isGetter': isGetter,
if (isField != null) 'isField': isField,
if (isMethod != null) 'isMethod': isMethod,
if (isStatic != null) 'isStatic': isStatic,
});
}) : this.fromJson(LazyMap(
[
if (isAbstract != null) 'isAbstract',
if (isClass != null) 'isClass',
if (isGetter != null) 'isGetter',
if (isField != null) 'isField',
if (isMethod != null) 'isMethod',
if (isStatic != null) 'isStatic',
],
(key) => switch (key) {
'isAbstract' => isAbstract,
'isClass' => isClass,
'isGetter' => isGetter,
'isField' => isField,
'isMethod' => isMethod,
'isStatic' => isStatic,
_ => null,
}));

/// Whether the entity is abstract, meaning it has no definition.
bool get isAbstract => node['isAbstract'] as bool;
Expand Down Expand Up @@ -146,9 +194,14 @@ extension type QualifiedName.fromJson(String string) {
extension type Query.fromJson(Map<String, Object?> node) {
Query({
QualifiedName? target,
}) : this.fromJson({
if (target != null) 'target': target,
});
}) : this.fromJson(LazyMap(
[
if (target != null) 'target',
],
(key) => switch (key) {
'target' => target,
_ => null,
}));

/// The class to query about.
QualifiedName get target => node['target'] as QualifiedName;
Expand Down
7 changes: 7 additions & 0 deletions pkgs/dart_model/lib/src/json_buffer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ class LazyMap with MapMixin<String, Object?> implements Map<String, Object?> {

LazyMap(Iterable<String> keys, this.lookup) : _keys = keys.toList();

const LazyMap.empty()
: _keys = const [],
lookup = _emptyLookup;

/// So we can create a const empty map.
static Object? _emptyLookup(_) => null;

@override
Iterable<String> get keys => _keys;

Expand Down
Loading