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

unroll/optimize some JavaConversions #1326

Closed
wants to merge 1 commit into from

Conversation

@ryan-williams
Copy link
Member

ryan-williams commented Dec 25, 2016

A .contains instead of a .containsKey is the main thing here; the latter does an O(n) conversion only to use a worse Java method (which takes Object, losing type-checking).

The others seem like they should save an O(n) on a .map by going through java/scala iterators instead of collections.

@AmplabJenkins
Copy link

AmplabJenkins commented Dec 25, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1706/
Test PASSed.

Copy link
Member

fnothaft left a comment

Awesome! Looks good @ryan-williams. I hadn't been aware of those issues WRT conversions—frankly, I don't use containsKey much—but do you have a link that talks about this more?

@ryan-williams
Copy link
Member Author

ryan-williams commented Dec 25, 2016

wrt containsKey, I just noticed I was getting failures in a branch on my fork where I swapped in a value-class ContigName for String in a few places, including in the SequenceDictionary class, and SequenceDictionary.containsRefName started returning false-negatives, because it was looking for Strings in a map whose keys were ContigNames; in a typed world / normal Scala an appropriate implicit would be applied to make sure you're not looking up a key in a map with the wrong type.

java.util.Map.containsKey takes an Object.

Worse still, in order for .containsKey to even be callable on the byName Map, an implicit JavaConversions member was being applied, so it was doubly suboptimal.

While fixing that, I commented out the JavaConversions import to audit the other implicit uses of it in the file, and thought I may as well change the other instances for the reason I described previously: better to .map and convert between java⟺scala on iterators and then materialize as a java x

Copy link
Member

heuermh left a comment

LGTM

@fnothaft
Copy link
Member

fnothaft commented Dec 26, 2016

@ryan-williams oh, that makes tons of sense. When this first came in, I just thinking to myself that I wasn't familiar with the containsKey method on any of the Scala collection classes... I agree with your point WRT converting between Scala and Java using iterators. That should probably be our best practice as long as we're not going to Avro-land (where java.lang.List rules the day) or to certain areas in htsjdk (where nothing is consistent).

@fnothaft
Copy link
Member

fnothaft commented Dec 26, 2016

Thanks @ryan-williams! I've manually merged as 2522760.

@fnothaft fnothaft closed this Dec 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.