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.

Sign up for GitHub

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

Jump to bottom

Response Cache v1 #3209

Merged
merged 51 commits into from
Sep 14, 2021
Merged

Response Cache v1 #3209

merged 51 commits into from
Sep 14, 2021

Conversation

rmccorm4
Copy link
Collaborator

@rmccorm4 rmccorm4 commented Aug 6, 2021 •
edited
Loading

Threading

  • single-threaded implementation
  • thread-safe implementation

Unit Tests

  • simple unit tests
  • multi-threaded unit tests
  • test LRU ordering

Memory

  • CPU memory only

Clean Up

  • Triton logging instead of custom macro
  • More fine-grained locking?

@rmccorm4
Add skeleton response cache class
29a1efa
@rmccorm4
update imports in response_cache.cc
96e57a8
@rmccorm4
Add skeleton for Hash() function
e4db6e9
@rmccorm4
add basic LRU logic
44b7bdd
@rmccorm4
rename cache_size -> total_size
70843d1
@rmccorm4
remove LookupStrict for now
4db69b2
@rmccorm4
add TODOs related to managed_buffer alloc/dealloc handling
f38d04f
@rmccorm4
more managed_buffer / ptr comments
e2add6d
@rmccorm4
fix comment on lru list ordering
7b61e9a
@rmccorm4
fix Update function definition
d53b140
@rmccorm4
add class prefix to response_cache.cc definitions
6e0c46c
@rmccorm4
create simple Output struct to use for caching response outputs
1491240
@rmccorm4
Create a unit test and start getting it to compile successfully
4515cd3
@rmccorm4
Add more details to Hash() function, add helper function BuildCacheEn…
ce0222e 
…try for constructing cache entry from InferenceResponse during Insert()
@rmccorm4
Add note about memory_type in output DataBuffer()
032aa3c
@rmccorm4
Add freeing cache entry buffers in destructor, comment out DataBuffer…
8dc5b79 
…() to compile until linker errors resolved
rmccorm4
rmccorm4 commented Aug 12, 2021
View reviewed changes
src/core/response_cache.cc Outdated Show resolved Hide resolved
rmccorm4
rmccorm4 commented Aug 12, 2021
View reviewed changes
src/core/response_cache.cc Outdated Show resolved Hide resolved
@rmccorm4
remove const when modifying attributes of output in Evict()
7cec457
@rmccorm4
Add missing .cc definitions for InferenceRequest/Response in unit tes…
26bac9f 
…t for compilation
@rmccorm4
Add memory hdrs/srcs to response cache test in CMakeLists.txt
83fb791
@rmccorm4
Add placeholders for infer request/response srcs in CMakeLists.txt if…
499b4cb 
… needed later
@rmccorm4
Add test for hashing InferenceRequests
ae12f2a
@rmccorm4
Get around InferenceBackend dependency in request.ModelName() by usin…
1af069f 
…g global const for backend name
@rmccorm4
add sample data in input request
db722cb
@rmccorm4
Hash request input buffers and validate through unit test
d0f3dbb
@rmccorm4
add start of BuildInferenceResponse()
20e397e
@rmccorm4
Compile BuildInferenceResponse() successfully
53ea3a2
@rmccorm4
Start testing cache lookup and insertion, failing on insertion
0850c84
@rmccorm4
Full test validating entry can be insert into cache, looked up, and r…
081a0a6 
…etrieved with the correct output values compiled and passed
deadeyegoodwin
deadeyegoodwin suggested changes Aug 30, 2021
View reviewed changes
src/core/response_cache.h Outdated Show resolved Hide resolved
src/core/response_cache.h Outdated Show resolved Hide resolved
src/core/response_cache.h Outdated Show resolved Hide resolved
src/core/response_cache.h Outdated Show resolved Hide resolved
src/core/response_cache.h Show resolved Hide resolved
src/core/response_cache.h Outdated Show resolved Hide resolved
@rmccorm4
Address most of first round review feedback
400af2d
@rmccorm4
Add handling for non-contiguous input buffers with DataBufferCount, a…
3f32a6a 
…nd add helpers to clean up hashing inputs
@rmccorm4
Use RETURN_IF_ERROR
8d30725
@rmccorm4
Move memcpy out of mocked AllocateDataBuffer, and add memcpy to Build…
1f6fbe2 
…InferenceResponse as expected
@rmccorm4
Use GTest macros instead of assertions
60191ae
@rmccorm4
Use ASSERT instead of EXPECT macros in gtest
232d96b
@rmccorm4
Use ImmutableInputs() instead of OriginalInputs() for hashing
fdf3d0b
@rmccorm4
Add mock PrepareForInference(), remove redundant status temp vars
00dc47f
@rmccorm4
check memory types on buffers to assert CPU only, run clang format
98c9014
@rmccorm4
Add mutexes to header, need to add synchronization in .cc implementat…
5c71516 
…ions
@rmccorm4
Add simple lock guards to most functions except for Constructor/Destr…
235c415 
…uctor/Hashing, could likely be improved to more fine-grained locking
@rmccorm4
add example thread creation for debugging
ac70956
@rmccorm4
Add parallel tests, add LRU test
b4012d2
GuanLuo
GuanLuo reviewed Sep 9, 2021
View reviewed changes
Copy link
Contributor

@GuanLuo GuanLuo left a comment

Choose a reason for hiding this comment

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

Looks good, just have some minor comments

src/core/response_cache.cc Show resolved Hide resolved
src/test/response_cache_test.cc Outdated Show resolved Hide resolved
@rmccorm4 rmccorm4 marked this pull request as ready for review September 13, 2021 17:34
@rmccorm4 rmccorm4 merged commit aa05498 into main Sep 14, 2021
@deadeyegoodwin deadeyegoodwin deleted the rmccormick-response-cache-v1 branch September 14, 2021 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuanLuo GuanLuo GuanLuo approved these changes

@deadeyegoodwin deadeyegoodwin deadeyegoodwin approved these changes

@CoderHam CoderHam Awaiting requested review from CoderHam

@tanmayv25 tanmayv25 Awaiting requested review from tanmayv25

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rmccorm4 @deadeyegoodwin @GuanLuo