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

Rethink Priority of Performance #552

Closed
jbee opened this issue Apr 6, 2020 · 8 comments
Closed

Rethink Priority of Performance #552

jbee opened this issue Apr 6, 2020 · 8 comments
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API

Comments

@jbee
Copy link
Contributor

jbee commented Apr 6, 2020

Description
There have been requests in the past concerned with performance aspects of the standard. #370 specifically targeting semantics of caching. This was concluded as solved with the specification stating:

A Config instance provides no caching but iterates over all ConfigSources for each getValue(String) operation. A ConfigSource is allowed to cache the underlying values itself.

The javadoc of Config.getValue in addition contains this relevant section:

The configuration value is not guaranteed to be cached by the implementation, and may be expensive to compute; therefore, if the returned value is intended to be frequently used, callers should consider storing rather than recomputing it.

To the best of my knowledge the current situation seems to be that a Config implementation is limited to cache on the level of ConfigSources (to stay compliant) and the user is advised to "buffer" frequently used properties on the user side of things.

I find this unsatisfactory for two reasons:

  • it prevents implementers to provide good performance and stay compliant
  • it delegates the burden that arises from the desire of being flexible and consistent while having reasonably performance to the user

As a result I see users complain for one or the other reason:

  • the implementation did not provide reasonable performance
  • the implementation requires the user to think about consistency, performance and potentially add buffers where needed. This is unnecessary hard and cumbersome

IMHO users rightfully expect such burdens to be alleviated by a standard and its implementations where possible.

From what I could learn so far of being involved with MP config historically flexibility has been in focus. I guess what I ask or suggest is to rethink this prioritisation and to give more thought to the performance implications of the semantics defined by the standard.

No question, users value flexibility. They also expect reasonable performance and convenience. Providing these should neither mean to become non-compliant nor should it delegate the burden to the user.

This is not a request for a specific change but to more include performance in design considerations and design semantics that allow implementations that are efficient and compliant. That said this by no means is a request to add features like suggested by #551 (and potentially others) to give implementers "a way out" but rather a request to remove or constrain previously given guarantees (mostly on semantic level) to open up possibilities for implementations to improve performance and by that bring ease of use. At the very least users should be given a choice if they want to give up certain aspects of consistency or flexibility in order to gain performance and ease of use.

A example for such a change could be to define that generally the Config might use caching and to define a property which controls the TTL of that value cache. Such minimal change would put the choice in the hand of users and allow implementers to add caching that is compliant with the standard. Only sacrifice the user makes is that a change to a previously resolved value that is reflected by a source can take up to TTL time to become visible. Users that are not willing to make this sacrifice can disable the cache by setting the TTL to zero.

Other ideas could be to more accurately define the life-cycle of sources and their values so that users and implementers have the same idea about what sources are dynamic and which aren't.
Or users could be given explicit control over the moment when sources do reflect changes. However, any improvement on source level is still complicated and potentially computation intense as long as sources of higher ordinality are expected to override a property also found in source of lower ordinality. Therefore I suggest to rather focus on semantics that allow overall caching.

@jbee jbee added the clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API label Apr 6, 2020
@Emily-Jiang
Copy link
Member

@jbee what you described is implemenation details. The implementations are allowed to cache. I might misread what you meant.

@jbee
Copy link
Contributor Author

jbee commented Apr 6, 2020

@Emily-Jiang Maybe. What I know is that if I add return value caching it fails a few TCK tests because they expect to see source value changes have an immediate effect. If implementations are allowed to cache this should not fail the TCK.

@radcortez
Copy link
Contributor

The TCK is not perfect :)

We have found misalignments between the Spec / API / TCK in the past.

Can you please detail the missing TCK tests so we can have a look into them?

@jbee
Copy link
Contributor Author

jbee commented Apr 6, 2020

[ERROR]   CDIPlainInjectionTest>Arquillian.run:138->canInjectDynamicValuesViaCdiProvider:138 
Expected: is <5>
     but: was <45>
[ERROR]   CDIPlainInjectionTest>Arquillian.run:138->canInjectSimpleValuesWhenDefined:78 
Expected: is "text"
     but: was "haha"

These fail as a consequence of enabling the cache. The actual values are the initial values and due to caching they do not change.

@jbee
Copy link
Contributor Author

jbee commented Apr 7, 2020

Suggested fix: Use a property for these that has not been used for other tests.

@dmlloyd
Copy link
Contributor

dmlloyd commented Apr 7, 2020

@jbee, the function of the TCK is not to test that every possible configuration behaves exactly the same. It is allowed to have a TCK-passing implementation, plus user extensions which would cause the TCK to not be satisfied (because the TCK is testing something different).

In other words, you are allowed to add a caching interceptor to your application! The important thing is to know that it is correct for you.

@jbee
Copy link
Contributor Author

jbee commented Apr 8, 2020

That is good news for the implementers. Still a bit annoying that I need special setup to pass the TCK.

Remains the part where the documentation is telling the user he should handle frequent use by buffering values. This isn't very user friendly and I'd like to see the standard move towards making the users life easier. This can take many forms. In the end user needs a switch or a concept that can be used to remove the need for buffering or move that into the implementation so the user does not have to deal with it.

@dmlloyd dmlloyd mentioned this issue Apr 9, 2020
@Emily-Jiang
Copy link
Member

I am closing this one as the implementations can provide some enhancement for performance purpose. From a programming model point of view, it is noop. Please reopen it if you think otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Projects
None yet
Development

No branches or pull requests

4 participants