ARROW-1627: New class to handle collection of BufferLedger(s) within … #1150

yufeldman · 2017-10-01T01:21:22Z

…AllocationManager

LowCostIdentityHashMap uses array only to store objects since key (BaseAllocator) is already included in the value (BufferLedger)
To make this class a bit more generic it uses values that implement ValueWithKeyIncluded interface that provides API : getKey()
BufferLedger provides implementation for it.
LowCostIdentityHashMap is not general purpose map it just provides some “map like” APIs to simplify switch over from IdentityHashMap.

jacques-n · 2017-10-01T16:39:33Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java

   * .netty.buffer package.
   */
-  public class BufferLedger {
+  public class BufferLedger implements ValueWithKeyIncluded {


Let's make ValueWithKeyIncluded be generic so it can return BaseAllocator in this case.

jacques-n · 2017-10-01T16:40:02Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

+  /*
+   * The internal data structure to hold values.
+   */
+  transient Object[] elementData;


Let's make all the fields private.

Also, no reason to use transient as we aren't using Java serialization.

jacques-n · 2017-10-01T16:40:35Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

+   * differentiate a literal 'null' from an empty spot in the
+   * map.
+   */
+  private static final Object NULL_OBJECT = new Object();  //$NON-LOCK-1$


Remove strange comment

Also, let's just not support not null values. Should simplify things.

jacques-n · 2017-10-01T16:42:39Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

+   *         {@code false} otherwise.
+   */
+  public boolean containsKey(Object key) {
+    if (key == null) {


Let's just throw exception if use passes in null for key.

jacques-n · 2017-10-01T16:44:23Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

+   * @return {@code true} if this map contains the specified value,
+   *         {@code false} otherwise.
+   */
+  public boolean containsValue(Object value) {


Let's make this a typed interface. E.g. containsValue(V value)

I know we don't follow Map iface, but just a note that in Map iface it is:
boolean containsValue(Object value)
If we follow the same pattern of "typing" we may also consider do it for other methods (get, remove, containsKey)

jacques-n · 2017-10-01T16:44:47Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

+   */
+  public boolean containsValue(Object value) {
+    if (value == null) {
+      value = NULL_OBJECT;


As above, let's throw illegal if user tries to look for null value.

jacques-n · 2017-10-01T16:44:59Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

+   * @return the value of the mapping with the specified key.
+   */
+  public V get(Object key) {
+    if (key == null) {


disallow nulls

jacques-n · 2017-10-01T16:45:32Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

+   *         {@code null} if there was no such mapping.
+   */
+  public V put(V value) {
+    Object _key = value.getKey();


Reject an insertion of null here.

jacques-n · 2017-10-01T16:46:23Z

java/memory/src/main/java/org/apache/arrow/memory/ValueWithKeyIncluded.java

+ * Helper Iface to generify a value to be included in the map where
+ * key is part of the value
+ */
+public interface ValueWithKeyIncluded {


as requested above, let's make this a generic interface so getKey is typed.

jacques-n · 2017-10-01T16:48:23Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java

  private final int size;
  private final UnsafeDirectLittleEndian underlying;
-  private final IdentityHashMap<BufferAllocator, BufferLedger> map = new IdentityHashMap<>();
+  private final LowCostIdentityHasMap<BufferLedger> map = new LowCostIdentityHasMap<>();


Let's add comment here on why we use this structure.

jacques-n

One small comment, otherwise looks good.

jacques-n · 2017-10-01T23:27:56Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

 * @param <V>
 */
-public class LowCostIdentityHasMap<V extends ValueWithKeyIncluded> {
+public class LowCostIdentityHasMap<K, V extends ValueWithKeyIncluded<K>> {


No need to declare K since V already declares it.

wesm · 2017-10-03T13:01:48Z

Can you please rebase? I just rebased master after 0.7.1 release

jacques-n

One general comment/question. Otherwise looks good.

jacques-n · 2017-10-03T16:12:43Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

Isn't there a way to make this typed without requiring two fields in the class type definition?

The only thing that can be done is : public V get(K key)
But in this case it is quite detached from "K" defined in
ValueWithKeyIncluded

Hmmm - it does not like angle brackets in comments:
public <K> V contains(K key)

jacques-n · 2017-10-03T16:13:00Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

same question as above

The only thing that can be done is : public V get(K key)
But in this case it is quite detached from "K" defined in
ValueWithKeyIncluded

Hmmm - it does not like angle brackets in comments:
public <K> V get(K key)

jacques-n · 2017-10-04T19:28:02Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

we can remove this function, right?

jacques-n · 2017-10-04T19:29:13Z

java/memory/src/main/java/org/apache/arrow/memory/LowCostIdentityHasMap.java

should this be equals() since we're comparing values?

not really - IdentityHashMap compares references for both keys and values. Unless we want to change the behavior

jacques-n

Lgtm. +1 Thanks @yufeldman!

…AllocationManager LowCostIdentityHashMap uses array only to store objects since key (BaseAllocator) is already included in the value (BufferLedger) To make this class a bit more generic it uses values that implement ValueWithKeyIncluded interface that provides API : getKey() BufferLedger provides implementation for it. LowCostIdentityHashMap is not general purpose map it just provides some “map like” APIs to simplify switch over from IdentityHashMap. ARROW-1627: Small improvement in hashing function. ARROW-1627: Addressing review comments

wesm · 2017-10-05T03:13:48Z

Merging, thanks for your contribution @yufeldman!

…AllocationManager LowCostIdentityHashMap uses array only to store objects since key (BaseAllocator) is already included in the value (BufferLedger) To make this class a bit more generic it uses values that implement ValueWithKeyIncluded interface that provides API : getKey() BufferLedger provides implementation for it. LowCostIdentityHashMap is not general purpose map it just provides some “map like” APIs to simplify switch over from IdentityHashMap. Author: Yuliya Feldman <yuliya@dremio.com> Closes apache#1150 from yufeldman/ARROW-1627-master and squashes the following commits: dcd5e29 [Yuliya Feldman] ARROW-1627: New class to handle collection of BufferLedger(s) within AllocationManager LowCostIdentityHashMap uses array only to store objects since key (BaseAllocator) is already included in the value (BufferLedger) To make this class a bit more generic it uses values that implement ValueWithKeyIncluded interface that provides API : getKey() BufferLedger provides implementation for it. LowCostIdentityHashMap is not general purpose map it just provides some “map like” APIs to simplify switch over from IdentityHashMap.

sadikovi · 2017-11-29T02:44:27Z

@wesm Should it be LowCostIdentityHasMap -> LowCostIdentityHashMap as class name?

yufeldman · 2017-11-29T02:54:26Z

Good catch - spelling error

sadikovi · 2017-11-29T02:56:02Z

Would it be okay to just open small PR without JIRA to fix this?

JIRA: https://issues.apache.org/jira/browse/ARROW-1869 This PR fixes spelling error in class name for `LowCostIdentityHashMap`. Follow-up for #1150. Author: Ivan Sadikov <ivan.sadikov@team.telstra.com> Closes #1372 from sadikovi/fix-low-cost-identity-hash-map and squashes the following commits: e3529f6 [Ivan Sadikov] fix low cost identity hash map name

…AllocationManager LowCostIdentityHashMap uses array only to store objects since key (BaseAllocator) is already included in the value (BufferLedger) To make this class a bit more generic it uses values that implement ValueWithKeyIncluded interface that provides API : getKey() BufferLedger provides implementation for it. LowCostIdentityHashMap is not general purpose map it just provides some “map like” APIs to simplify switch over from IdentityHashMap. Author: Yuliya Feldman <yuliya@dremio.com> Closes apache#1150 from yufeldman/ARROW-1627-master and squashes the following commits: dcd5e29 [Yuliya Feldman] ARROW-1627: New class to handle collection of BufferLedger(s) within AllocationManager LowCostIdentityHashMap uses array only to store objects since key (BaseAllocator) is already included in the value (BufferLedger) To make this class a bit more generic it uses values that implement ValueWithKeyIncluded interface that provides API : getKey() BufferLedger provides implementation for it. LowCostIdentityHashMap is not general purpose map it just provides some “map like” APIs to simplify switch over from IdentityHashMap.

JIRA: https://issues.apache.org/jira/browse/ARROW-1869 This PR fixes spelling error in class name for `LowCostIdentityHashMap`. Follow-up for apache#1150. Author: Ivan Sadikov <ivan.sadikov@team.telstra.com> Closes apache#1372 from sadikovi/fix-low-cost-identity-hash-map and squashes the following commits: e3529f6 [Ivan Sadikov] fix low cost identity hash map name

jacques-n reviewed Oct 1, 2017

View reviewed changes

asfgit force-pushed the master branch from 417cef5 to c905783 Compare October 3, 2017 12:59

yufeldman force-pushed the ARROW-1627-master branch from bcc4c71 to fada2b5 Compare October 3, 2017 16:04

jacques-n reviewed Oct 3, 2017

View reviewed changes

jacques-n reviewed Oct 4, 2017

View reviewed changes

jacques-n approved these changes Oct 5, 2017

View reviewed changes

yufeldman force-pushed the ARROW-1627-master branch from 6cf4810 to dcd5e29 Compare October 5, 2017 02:04

asfgit closed this in dc129d6 Oct 5, 2017

sadikovi mentioned this pull request Nov 29, 2017

ARROW-1869: [JAVA] Fix LowCostIdentityHashMap name #1372

Closed

asfimport mentioned this pull request Jul 8, 2018

[JAVA] Reduce heap usage(Phase 2) - memory footprint in AllocationManager.BufferLedger #17638

Closed

ARROW-1627: New class to handle collection of BufferLedger(s) within … #1150

ARROW-1627: New class to handle collection of BufferLedger(s) within … #1150

Uh oh!

Conversation

yufeldman commented Oct 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacques-n left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Oct 3, 2017

Uh oh!

jacques-n left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yufeldman Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yufeldman Oct 3, 2017 •

edited

Loading

sadikovi commented Nov 29, 2017 •

edited

Loading