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

Jump to bottom

HY-4981 Memory audit #17

Merged
merged 12 commits into from
Jul 31, 2017
Merged

HY-4981 Memory audit #17

merged 12 commits into from
Jul 31, 2017

Conversation

robbecker-wf
Copy link
Member

@robbecker-wf robbecker-wf commented Jul 26, 2017 •
edited
Loading

Overview

This repo needs a memory audit.

Changes

Code was reviewed for memory retention issues. 1 issue was found and fixed.
onScroll StreamSubscriptions are now cancelled for each of the 3 rulers when detecting font load events (only affects browsers that don't have the Font Face API)

Testing / +10

  • Fire up the test example: pub serve test and visit http://localhost:8080/unload.html
  • Start a performance timeline recording and click the "load a bunch of fonts" button
  • Click the GC button, and then end the recording. You should see the # of listeners (yellow line) drop back down at the end of the recording.
    image

@aviary2-wf
Copy link

aviary2-wf commented Jul 26, 2017 •
edited by aviary-wf
Loading

Raven

Number of Findings: 0

@rmconsole-wf
Copy link
Contributor

rmconsole-wf commented Jul 26, 2017 •
edited
Loading

General Information

Ticket(s):

Code Review(s): #17

Additional Information

Reviewers: robbecker-wf, patkujawa-wf, georgelesica-wf, smaifullerton-wk
Watchlist Notifications: None

	When this pull is merged I will add it to the following release:
	Version: font_face_observer 2.0.1
	Release Ticket(s): HY-4744, HY-4745

Last updated on Monday, July 31 01:41 PM CST

@semveraudit-wf
Copy link

semveraudit-wf commented Jul 28, 2017 •
edited
Loading

Public API Changes

No changes to the public API found for commit 915c563

Showing results for 915c563

Powered by semver-audit-service. Please report any problems by filing an issue.
Browse public API.

georgelesica-wf
georgelesica-wf reviewed Jul 31, 2017
View reviewed changes
lib/src/ruler.dart Outdated
@@ -35,6 +36,9 @@ class Ruler {
/// The test string to use when measuring
String text;

List<StreamSubscription<Event>> _subscriptions =
new List<StreamSubscription<Event>>();
Copy link

Choose a reason for hiding this comment

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

IIRC Dart style guide says do <StreamSubscription<Event>>[].

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

georgelesica-wf
georgelesica-wf reviewed Jul 31, 2017
View reviewed changes
lib/src/ruler.dart
@@ -134,12 +138,33 @@ class Ruler {

/// Register a callback to be called when the ruler is resized
void onResize(_OnScrollCallback callback) {
_collapsible.onScroll.listen((Event _) {
_subscriptions.add(_collapsible.onScroll.listen((Event _) {
Copy link

Choose a reason for hiding this comment

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

Two questions...

  1. Isn't this going to keep accumulating subscriptions as resize events occur? That seems dangerous...
  2. You could also use manageStreamSubscription or listenToStream here since it looks like these only get canceled once the dispose method is called

Copy link
Member Author

Choose a reason for hiding this comment

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

re #1: onResize isn't called for each resize event ... it's registering that passed in callback to use when a resize happens.
re #2: this library does not use w_common disposable. It just handles this 1 case manually.

georgelesica-wf
georgelesica-wf reviewed Jul 31, 2017
View reviewed changes
lib/src/ruler.dart
_reset();
}

/// Clean up when this ruler is no longer used
void dispose() {
Copy link

Choose a reason for hiding this comment

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

Is this object actually "disposable"? If so, probably want to use onDispose. If not, should it be? If the answer is "no", disregard my comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not using w_common disposable .. but I could rename this to reduce confusion of people thinking that it may be.

Copy link

Choose a reason for hiding this comment

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

Cool, yeah, I would personally prefer a different name, but only if there's another name that makes sense in this particular context.

@georgelesica-wf
Copy link

Couple comments.

@georgelesica-wf
Copy link

+1

@smaifullerton-wk
Copy link

+10

  • Fire up the test example: pub serve test and visit http://localhost:8080/unload.html
  • Start a performance timeline recording and click the "load a bunch of fonts" button
  • Click the GC button, and then end the recording. You should see the # of listeners (yellow line) drop back down at the end of the recording.

image

@patkujawa-wf
Copy link

Quality Review Approval: +1

  • Testing instruction
  • Dev +1's
  • Dev/QA +10 with detail of what was tested
  • All unit tests pass
  • Unit test created/updated
  • Rosie has run and reports properly the release the ticket will be included in

@Workiva/release-management-p for merge into master

@rmconsole-wf
Copy link
Contributor

+1 from RM

@rmconsole-wf rmconsole-wf merged commit 12fb799 into master Jul 31, 2017
@rmconsole3-wf rmconsole3-wf deleted the memory_audit branch July 31, 2017 18:41
@rmconsole-wf rmconsole-wf mentioned this pull request Jul 31, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgelesica-wf georgelesica-wf georgelesica-wf left review comments

Assignees
No one assigned
Labels
Merge Requirements Met RM Ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@robbecker-wf @aviary2-wf @rmconsole-wf @semveraudit-wf @georgelesica-wf @smaifullerton-wk @patkujawa-wf