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

Make Inputs class an interface which can be implemented per vajram #166

Closed
RamAnvesh opened this issue Aug 15, 2023 · 3 comments · Fixed by #327
Closed

Make Inputs class an interface which can be implemented per vajram #166

RamAnvesh opened this issue Aug 15, 2023 · 3 comments · Fixed by #327
Assignees
Labels
performance Performance enhancement related
Milestone

Comments

@RamAnvesh
Copy link
Collaborator

RamAnvesh commented Aug 15, 2023

Currently, Inputs is a final class encapsulating a map. This means every time we need to access any single input from the class we need to a ImmutableMap lookup. While an ImmutableMap lookup is O(1) and might be acceptable in common scenarios, this can add up to consume some significant (single digit %) CPU in a framework like Krsytal where we process thousands of requests per second and each request accesses inputs multiple times. While the ImmutableMap lookup might be inevitable in some scenarios, it can be avoided in other scenarios where the call is being made from inside vajram code which is data type and input schema aware, rather than krystex code which is data type and input schema unaware.

To take advantage of this difference, we can do the following:
We make Inputs class an interface and let the vajram-codegen auto-generate an implementation of Inputs. This implemented class will have the ability to access individual inputs without map look-ups if the caller knows the input name at compile time. Even in cases where the inputs are accessed from krystex, we can autogenerate a switch-cased input accessor which could be faster than a hash map lookup (This needs to be verified and quantified. Also, if switching over strings (input names) is slower, we can consider assigning a unique int id to each input, and use that to refer to inputs instead. This will definitely improve performance of random access of inputs from krystex since switching over integers is faster than switching over strings and looking up hashmaps - this improvement needs to be quantified as well.
Refs:
https://stackoverflow.com/questions/27993819/hashmap-vs-switch-statement-performance
https://web.archive.org/web/20180818151450/http://java-performance.info/string-switch-implementation/
https://stackoverflow.com/questions/12020048/how-does-javas-switch-work-under-the-hood
https://www.artima.com/articles/control-flow-in-the-java-virtual-machine
)

The implication of this is that Inputs equality can get affected. Currently two Inputs are equal if they have the same key value pairs. But with auto-generated Inputs implementations, this behaviour might no be possible to achieve. I don't think Krystal relies on this behaviour anywhere, so this change should be fine, but this has to be verified first.

@RamAnvesh RamAnvesh added the performance Performance enhancement related label Aug 15, 2023
@vinisha-parwal
Copy link
Contributor

How will we dedupe the request if we are not checking the equality? Even in case of compute request.

@RamAnvesh
Copy link
Collaborator Author

How will we dedupe the request if we are not checking the equality? Even in case of compute request.

No no equality will be checked. It is just that it is not straight forward and needs to be designed as to how we can get consistent equality in auto generated code.
.

@RamAnvesh RamAnvesh assigned Jeel11 and unassigned vinisha-parwal Aug 22, 2023
@RamAnvesh RamAnvesh assigned RamAnvesh and unassigned Jeel11 and RamAnvesh May 7, 2024
@RamAnvesh RamAnvesh added this to the v9 milestone Feb 2, 2025
@RamAnvesh RamAnvesh linked a pull request Feb 6, 2025 that will close this issue
@RamAnvesh
Copy link
Collaborator Author

This is done.
Switch casing turned out not to be needed. Instead we unified facet definition and facet spec into a common hierarchy called Facet, and each facet has one lambda each for getting and setting a facet's value from a FacetValues object(these are is code generated).
Also facets are immutable singleton objects being passed around the graph.. so we are able to directly invoke the lambdas - even from the framework code in which case the is no conditional execution (no switch case). Instead there is a lambda invocation and a type cast. Since there is no conditional bracing in either of them, we can get better cpu throughput due to better instruction pipelining and close to perfect branch prediction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance enhancement related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants