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

Better lazy resolution #91

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Better lazy resolution #91

wants to merge 7 commits into from

Conversation

ctrueden
Copy link
Member

@ctrueden ctrueden commented Jul 1, 2014

MERGE ONLY IN CONCERT WITH THE OTHER 3.0.0 BRANCHES.

This improves the ObjectIndex lazy object resolution feature to only resolve objects as needed based on the type. Previously, as soon as get(Class) or getAll() was called, all pending objects would be resolved. Now we call the new LazyObjects#getType() method to see if the type of the Collection is even worth bothering about.

See also issue #90.

@ctrueden
Copy link
Member Author

ctrueden commented Jul 2, 2014

I have come to the conclusion that this approach is simply not good enough in all scenarios. We need to add an ObjectIndex#addLater(LazyObjects, Class) method signature that lets you explicitly specify the type of the objects which will be resolved. Otherwise, the behavior will never be good enough.

I will work on that as soon as I can...

@ctrueden
Copy link
Member Author

ctrueden commented Jul 7, 2014

I reworked the branch to add a getType() method to LazyObjects which returns the type of the to-be-resolved objects. This information is then used to decide whether to resolve the objects when ObjectIndex#get(Class) is called.

It is working fine, but is a breaking change from the SJC 2.x.x API, so bumps the snapshot version to 3.0.0. It should be merged only in concert with other breaking SJC branches, particularly robust-io (which is not yet complete).

@ctrueden ctrueden added this to the 3.0.0 milestone Jul 25, 2014
ctrueden and others added 7 commits May 12, 2015 16:47
Specifically, we test that lazy object resolution works in conjunction
with a call to getAll(), to ensure that pending objects get resolved.
The LazyObjects instance will definitely return a collection of objects
of type PT, not just Object. This distinction is important now that the
ObjectIndex is more selective about which pending objects it resolves.
This breaks backwards compatibility, so bumps the development snapshot
version to 3.0.0.

The rationale is to make it possible for the ObjectIndex to better
discriminate which objects should be resolved for a particular call to
ObjectIndex#get(Class). The current behavior is to resolve _all_ pending
objects with _any_ call to get(Class), which is generally suboptimal.
If a pending object will not be of a type compatible with the
requested type, then skip resolution of that pending object.

This improves performance of the get(Class) and getAll() methods in
cases where there are a large number of pending objects in the index.

It also helps to eliminate difficulties like those in issue #90.
This tests that the addLater method works in conjunction with calls to
get(Class). The logic is nontrivial because we only want to resolve
pending objects when the types are compatible with the requested one.
The filterInstances method should not alter the return type of the input
list. The purpose of that method is to potentially remove some elements
from the input list -- nothing more -- so it should still return a
List<PT>, not something else.
Instead of maintaining our own list of singletons, we should be using
the ObjectService.
@ctrueden ctrueden force-pushed the better-lazy-resolution branch from cbc827c to d6a7eba Compare May 12, 2015 22:02
@ctrueden
Copy link
Member Author

While rebasing this branch, I noticed a problem with AbstractSingletonService as written. It now has a hard map of instances (due to 1b245c1). I think we should reconsider this—perhaps the cache should be weak and lazy instead. And unit tests, unit tests.

@ctrueden ctrueden force-pushed the master branch 3 times, most recently from 53b6733 to 3dc99c9 Compare November 7, 2023 18:37
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