Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

perf(scope): misc optims #610

Closed
wants to merge 2 commits into from
Closed

perf(scope): misc optims #610

wants to merge 2 commits into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Feb 25, 2014

2 optimizations:

  • children created from createChild() are always last in llist,
  • _mapEqual() can stop comparing values on first mismatch

& some minor cleanup

@vicb vicb added cla: yes and removed cla: no labels Feb 25, 2014
}
_mapEqual(Map a, Map b) => a.length == b.length
? a.keys.every((k) => b.containsKey(k) && a[k] == b[k])
: false;
Copy link

Choose a reason for hiding this comment

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

a.length == b.length && a.keys.every((k) => b.containsKey(k) && a[k] == b[k])

Btw, what is wrong with a regular a == b?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a == b would return true only if a and b are the same map, not if they have the same content.

Thanks for the short version, will update.

Copy link

Choose a reason for hiding this comment

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

Ah, yes. For some reason I thought == was value based by default in Dart since there is no equals method.

b.containsKey(k) && a[k] == b[k] seems rather inefficient... but I can't really see any better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

There seems to be a MapEquality in the collections package. Why not use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks very generic and then may be overkill ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chalin
Copy link
Contributor

chalin commented Feb 25, 2014

Like @ronag I thought == over Map implemented map equality rather than just identical().
@ronag where did you find MapEquality? Is it the one in the "Deprecated helper libraries"? If so, then it should likely not be used.

child._prev = prev;
if (prev == null) _childHead = child; else prev._next = child;
if (next == null) _childTail = child; else next._prev = child;
if (prev == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the original code much more readable. Can you revert this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted in the last commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops already merged... I'll re-submit the optim as it seems to have been reverted altogether with the style !

@vicb vicb closed this in 7f36a8e Feb 26, 2014
@ronag
Copy link

ronag commented Feb 26, 2014

@chalin: MapEquality is in the collections pub package.

@vicb
Copy link
Contributor Author

vicb commented Feb 26, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

4 participants