Skip to content
This repository was archived by the owner on Jan 20, 2022. It is now read-only.

Pluggable caches in FinalFlatMap#396

Merged
johnynek merged 11 commits intodevelopfrom
feature/branchF
Dec 6, 2013
Merged

Pluggable caches in FinalFlatMap#396
johnynek merged 11 commits intodevelopfrom
feature/branchF

Conversation

@ianoc
Copy link
Copy Markdown
Contributor

@ianoc ianoc commented Dec 5, 2013

Added an synchronous cache, pulled cache logic out thats used by the FFM. Allow ticks to trigger flushing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems like a magic number. Config? val?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this use the pool?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how the pool could use the pool to shutdown? it would be recursive right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it not safe to call shutdown from a thread that is created by the pool? If not, how do we ensure that?

Not a big deal, but if you don't want to tackle, add an issue, a link to it here and a comment explaining the concern about correctness and using the pool to shut itself down.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You could call shutdown, but this currently waits to ensure its shutdown before returning -- if we drop that part (awaitShutdown). Then we could do it inside the pool?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added an issue

#397

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about: Runtime.getRuntime().availableProcessors() as the default pool size?

http://stackoverflow.com/questions/4759570/finding-number-of-cores-in-java

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure

@ianoc
Copy link
Copy Markdown
Contributor Author

ianoc commented Dec 5, 2013

@johnynek poke

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

more comments here. So this branch happens when there IS a value for this key, right? Not there is confusing to me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like these. Can we replace with JavaConverters._ and use the .asJava .asScala syntax. Implicit conversions of this kind bite us.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(breakOut) magic is supposedly faster here:

import scala.collection.breakOut
flatMap { k => ... }(breakOut)

http://stackoverflow.com/questions/1715681/scala-2-8-breakout

johnynek added a commit that referenced this pull request Dec 6, 2013
Pluggable caches in FinalFlatMap
@johnynek johnynek merged commit 30c03b6 into develop Dec 6, 2013
@johnynek johnynek deleted the feature/branchF branch December 6, 2013 01:10
@johnynek
Copy link
Copy Markdown
Contributor

johnynek commented Dec 6, 2013

Thanks for pushing this through. Next up, see if we can get it upping Ashu's throughput?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants