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

Added phase 3 methods and test for remove bulk #91

Merged
merged 8 commits into from
Jul 26, 2017
Merged

Conversation

TylerADavis
Copy link
Collaborator

Resolves #61 . Added PrefetchBulk and RemoveBulk to cache manager and store. The new test for the store's RemoveBulk passes, as does cpplint. The code is ready to be looked over.

However, I did not include a test for prefetch bulk or the cache's remove bulk, as I don't see a way to check the success of these operations as the map that would need to be accessed to test this is private. Namely, from outside the class it is impossible to tell if an item has been prefetched or if it has been removed, as if it is not present the cache manager will simply fetch remotely. Do you have any recommendations on how to test these methods?

@TylerADavis TylerADavis requested a review from jcarreira July 13, 2017 19:04
@@ -106,6 +108,11 @@ typename ObjectStore<T>::ObjectStorePutFuture ObjectStore<T>::put_async(
return ObjectStorePutFuture();
}

template<class T>
void ObjectStore<T>::removeBulk(ObjectID first, ObjectID last) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should there be comments indicating that these are placeholder methods?

Copy link
Owner

Choose a reason for hiding this comment

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

Why isn't this declared as a virtual method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I have declared it virtual void etc. here as it was declared as such above? Or are you asking why I even have this section of code at all?

If the latter, I included it because there are two store classes (Fullcache store and RDMAObjectStore) that I feared would not compile as they would not have the removeBulk method implemented.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it should be declared as a virtual method. The store classes can define that method -- and throw "not implemented" exception.

Copy link
Collaborator Author

@TylerADavis TylerADavis Jul 18, 2017

Choose a reason for hiding this comment

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

Alright, I'll take that approach in the phase two pull request as well, and also apply it to get_async() and put_async().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realized that the other two store classes have actually not been getting compiled as there is no code that relies on them. As such, they are not up to date with the current interface. I think it may be best to simply open an issue to bring them up to date as opposed to fixing them here and in #81 . Do you agree?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've opened an issue for this in #95

@TylerADavis
Copy link
Collaborator Author

With the new behavior of cache remove (having it remove from the store), I should be able to test removeBulk(), but I'm still unsure how to test prefetchbulk() as there is no external indication of whether or not a cache item has been prefetched.

@jcarreira
Copy link
Owner

You can make the test check the latency of accessing an object. If it is local it should take less than 1us, if it is remote should be higher.

@TylerADavis
Copy link
Collaborator Author

That approach sounds good to me, I'll go ahead and try to implement it.

@TylerADavis
Copy link
Collaborator Author

TylerADavis commented Jul 19, 2017

The latest commit adds the tests for prefetch bulk and remove bulk. I ended up going with a cutoff time of 5µs as some prefetched gets took closer to that amount of time, with remote accesses taking order of magnitude more time.

Make check and cpplint pass.

@jcarreira
Copy link
Owner

Why does it take so long to get an object that is cached?

@TylerADavis
Copy link
Collaborator Author

I'm not sure, I'll look into where the time is spent today

@TylerADavis
Copy link
Collaborator Author

TylerADavis commented Jul 20, 2017

As an update, if my timing methodology is correct, then it takes ~80 ms (80,000 µs) to retrieve from the remote store (using store.get(i)) over TCP on this branch. This seems exceptionally high, as this is for four byte messages, but it makes sense as this branch does not have the TCP speed improvements (namely the disabling of Nagle's algorithm) that were merged into mac_compiling.

@TylerADavis
Copy link
Collaborator Author

TylerADavis commented Jul 20, 2017

As for the latency of retrieving an object in the local cache, it seems that it generally takes less than a microsecond (reported time elapsed is zero), but the first access takes 3 µs typically (and the second access takes 1 µs sometimes). This doesn't seem to be a result of having to wait for the prefetch to be pulled from remote storage, as I obtained the same result regardless of if the system waited .1 or 1 seconds before testing access latency.

I switched the timing to nanoseconds and found that the average access time is ~350 ns with a cache of 100 objects.

@jcarreira
Copy link
Owner

jcarreira commented Jul 21, 2017

The 80ms are in line with what I observed in my laptop before the Nagle changes.

Good, these last latencies for cached objects are much more in line with what we should observe (i.e., average <1us)

@jcarreira jcarreira merged commit 6f4ab75 into async_ops Jul 26, 2017
@jcarreira jcarreira deleted the phase_three branch August 17, 2017 04:54
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.

2 participants