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

Implement support for request priorities #666

Merged
merged 1 commit into from Sep 23, 2014

Conversation

3 participants
@lucasr
Contributor

lucasr commented Sep 23, 2014

Not sure if you've considered something like this in the past but here it is. This patch implements request priority support in Picasso. This feature can be especially handy when you have UIs with lots of images and you want to have tighter control over which images should load first. This is inspired by Volley's API.

It adds a new API in RequestCreator to define the request priority:

Picasso.with(context)
       .load("http://example.com/image.jpg")
       .priority(HIGH)
       .into(someImageView);

Priority can be LOW, NORMAL, HIGH, or IMMEDIATE. BitmapHunters compute their priority in the executor by taking the highest priority of its attached requests. For example, if a BitmapHunter has two attached Actions, one LOW and and one HIGH priority, its priority will be HIGH.

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

We have, but I think those might be too many priorities. This is the same problem as log levels. Everyone wants a lower log level or a higher log level because they abuse them.

I wanted only two, but I can probably be convinced for three. I don't think I can be convinced of four.

Collaborator

JakeWharton commented Sep 23, 2014

We have, but I think those might be too many priorities. This is the same problem as log levels. Everyone wants a lower log level or a higher log level because they abuse them.

I wanted only two, but I can probably be convinced for three. I don't think I can be convinced of four.

@JakeWharton

View changes

Show outdated Hide outdated picasso/src/main/java/com/squareup/picasso/BitmapHunter.java
@@ -69,6 +74,7 @@
BitmapHunter(Picasso picasso, Dispatcher dispatcher, Cache cache, Stats stats, Action action,
RequestHandler requestHandler) {
this.sequence = SEQUENCE_GENERATOR.addAndGet(1);

This comment has been minimized.

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

incrementAndGet()?

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

incrementAndGet()?

This comment has been minimized.

@lucasr

lucasr Sep 23, 2014

Contributor

Done.

@lucasr

lucasr Sep 23, 2014

Contributor

Done.

@JakeWharton

View changes

Show outdated Hide outdated picasso/src/main/java/com/squareup/picasso/PicassoExecutorService.java
@Override
public Future<?> submit(Runnable task) {
if (task == null) {
throw new NullPointerException();

This comment has been minimized.

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

AssertionError instead? Would this ever really happen?

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

AssertionError instead? Would this ever really happen?

This comment has been minimized.

@lucasr

lucasr Sep 23, 2014

Contributor

I was just being overcautious here. I don't think this is actually possible. Removed.

@lucasr

lucasr Sep 23, 2014

Contributor

I was just being overcautious here. I don't think this is actually possible. Removed.

@@ -266,6 +272,32 @@ Exception getException() {
return loadedFrom;
}
Priority getPriority() {

This comment has been minimized.

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

This is fairly expensive computationally. I think we could track this in a field and update it as requests are attached/detached from inside an Action. This would make computation incremental and access cheap (the latter of which is essential when the priority queue is calling compateTo multiple times).

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

This is fairly expensive computationally. I think we could track this in a field and update it as requests are attached/detached from inside an Action. This would make computation incremental and access cheap (the latter of which is essential when the priority queue is calling compateTo multiple times).

This comment has been minimized.

@lucasr

lucasr Sep 23, 2014

Contributor

Good point. I considered doing that but my assumption here was that the hasMultiple case is rare. Most of the time, it will just do:

if (action != null) {
    priority = action.getPriority();
}

But, yeah, updating the priority as actions get attached/detached is a bit more future-proof—even though it will add a bit of complexity to BitmapHunter. I'll try it.

@lucasr

lucasr Sep 23, 2014

Contributor

Good point. I considered doing that but my assumption here was that the hasMultiple case is rare. Most of the time, it will just do:

if (action != null) {
    priority = action.getPriority();
}

But, yeah, updating the priority as actions get attached/detached is a bit more future-proof—even though it will add a bit of complexity to BitmapHunter. I'll try it.

@@ -370,8 +389,12 @@ public Request build() {
if (centerInside && targetWidth == 0) {
throw new IllegalStateException("Center inside requires calling resize.");
}
if (priority == null) {
priority = Priority.NORMAL;

This comment has been minimized.

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

fetch requests should set their priority to LOW automatically. That doesn't happen here, but here is the only relevant place I can comment.

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

fetch requests should set their priority to LOW automatically. That doesn't happen here, but here is the only relevant place I can comment.

@JakeWharton

View changes

Show outdated Hide outdated picasso/src/main/java/com/squareup/picasso/RequestCreator.java
@@ -229,6 +230,16 @@ public RequestCreator config(Bitmap.Config config) {
}
/**
* Set the priority of this request.
* <p>
* This will affect the order in which the requests execute.

This comment has been minimized.

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

append "but does not guarantee it"

@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

append "but does not guarantee it"

This comment has been minimized.

@lucasr

lucasr Sep 23, 2014

Contributor

Done.

@lucasr

lucasr Sep 23, 2014

Contributor

Done.

@lucasr

This comment has been minimized.

Show comment
Hide comment
@lucasr

lucasr Sep 23, 2014

Contributor

@JakeWharton Fair enough. Let's start with just LOW, NORMAL, HIGH then. I think using only two priorities feels too "polarizing". Three gives enough flexibility for more complex cases.

Contributor

lucasr commented Sep 23, 2014

@JakeWharton Fair enough. Let's start with just LOW, NORMAL, HIGH then. I think using only two priorities feels too "polarizing". Three gives enough flexibility for more complex cases.

@lucasr

This comment has been minimized.

Show comment
Hide comment
@lucasr

lucasr Sep 23, 2014

Contributor

Updated branch:

  • Update BitmapHunter priority when requests get attached/detached. getPriority() is now simple getter.
  • Moar tests.
  • Moar API docs.
Contributor

lucasr commented Sep 23, 2014

Updated branch:

  • Update BitmapHunter priority when requests get attached/detached. getPriority() is now simple getter.
  • Moar tests.
  • Moar API docs.
@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Sep 23, 2014

Collaborator

👍 👍 👍

One of the more challenging things I'd like to figure out is a way to limit the number of threads on which LOW priority requests can execute. Something like half the threads (and then rounded down). I'd like to be able to execute 100 fetch() requests, for example, but still allow NORMAL and HIGH requests to start without having to wait.

Collaborator

JakeWharton commented Sep 23, 2014

👍 👍 👍

One of the more challenging things I'd like to figure out is a way to limit the number of threads on which LOW priority requests can execute. Something like half the threads (and then rounded down). I'd like to be able to execute 100 fetch() requests, for example, but still allow NORMAL and HIGH requests to start without having to wait.

JakeWharton added a commit that referenced this pull request Sep 23, 2014

Merge pull request #666 from lucasr/request-priority
Implement support for request priorities

@JakeWharton JakeWharton merged commit c21d17f into square:master Sep 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@felipecsl

This comment has been minimized.

Show comment
Hide comment
@felipecsl

felipecsl Sep 24, 2014

👍 well done @lucasr

👍 well done @lucasr

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