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

Reduce memory overhead caused by default collection sizes #4562

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

Legioth
Copy link
Member

@Legioth Legioth commented Aug 28, 2018

There are various collections that are usually not used at all or with
only a small number of items. Allocating the default sizes for such
collections wastes quite some memory: HashSet and HashMap allocate for
16 items by default, ArrayList for 10 items and IdentityHashMap for 32.

Reduces BasicElementView memory use from 429706 to 419682 bytes and the
memory use in BeverageBuddy with an edit dialog open from 262257 to
251569 bytes.


This change is Reviewable

@Legioth Legioth force-pushed the avoidDefaultCollections branch from f45378a to 8425a19 Compare August 28, 2018 10:38
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r1.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale

Copy link
Contributor

@pekam pekam left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 5 files at r1.
Reviewable status: 2 unresolved discussions, 1 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 155 at r1 (raw file):

        }

        componentEventData.computeIfAbsent(eventType, t -> new ArrayList<>())

This ArrayList could also have initial capacity.
Most commonly it will contain only one item, since it's rare to add multiple listeners for the same event type.


flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 356 at r1 (raw file):

listeners.containsKey(eventType)

This returns always true, (because of the above listenerList != null check), so what's the point of having it in here?

assert would make more sense if you want to include this check anyway.


flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementPropertyMap.java, line 151 at r1 (raw file):

        propertyListeners.add(listener);
        return () -> propertyListeners.remove(listener);

I guess removal could also be optimized like in ElementListenerMap?

But that's not as common case, with not as big benefits, so not blocking.

There are various collections that are usually not used at all or with
only a small number of items. Allocating the default sizes for such
collections wastes quite some memory: HashSet and HashMap allocate for
16 items by default, ArrayList for 10 items and IdentityHashMap for 32.

Reduces BasicElementView memory use from 429706 to 419682 bytes and the
memory use in BeverageBuddy with an edit dialog open from 262257 to
251569 bytes.
@Legioth Legioth force-pushed the avoidDefaultCollections branch from 8425a19 to 1be700a Compare August 30, 2018 13:26
Copy link
Member Author

@Legioth Legioth left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale


flow-server/src/main/java/com/vaadin/flow/component/ComponentEventBus.java, line 155 at r1 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

This ArrayList could also have initial capacity.
Most commonly it will contain only one item, since it's rare to add multiple listeners for the same event type.

Done.


flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java, line 356 at r1 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
listeners.containsKey(eventType)

This returns always true, (because of the above listenerList != null check), so what's the point of having it in here?

assert would make more sense if you want to include this check anyway.

Done.

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 5 issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MAJOR Binder.java#L765: Remove usage of generic wildcard type. rule
  2. MAJOR ElementListenerMap.java#L386: Rename "listeners" which hides the field declared at line 59. rule
  3. MINOR Binder.java#L709: "public" is redundant in this context. rule
  4. MINOR ComponentEventBus.java#L202: Remove useless curly braces around statement rule
  5. MINOR ElementListenerMap.java#L344: Move this method into "DomEventListenerWrapper". rule

Copy link
Contributor

@pekam pekam left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 2 stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants