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

[java] FuryPooledObjectFactory#classLoaderFuryPooledCache remove softValues #1632

Closed
MrChang0 opened this issue May 14, 2024 · 10 comments · Fixed by #1639
Closed

[java] FuryPooledObjectFactory#classLoaderFuryPooledCache remove softValues #1632

MrChang0 opened this issue May 14, 2024 · 10 comments · Fixed by #1639
Labels
enhancement New feature or request

Comments

@MrChang0
Copy link
Contributor

MrChang0 commented May 14, 2024

Is your feature request related to a problem? Please describe.

classLoaderFuryPooledCache with weakKeys,softValues,expireAfterAccess , values removed too frequently. In some case it will create too much fury.

Describe the solution you'd like

I'd like to remove softValues options. even weakKey can be removed too, just remain expireAfterAccess

Additional context

@MrChang0 MrChang0 added the enhancement New feature or request label May 14, 2024
@chaokunyang
Copy link
Collaborator

If we remove it, those classes will never be collected. In some hot-loading classes scenarios, it will may cause class leak

@chaokunyang
Copy link
Collaborator

chaokunyang commented May 14, 2024

Maybe we can resolve this in another way. Like let users call Fury to clear classes explicitly.

In this way, we can remove those weak/soft values

@MrChang0
Copy link
Contributor Author

MrChang0 commented May 14, 2024

If we remove it, those classes will never be collected. In some hot-loading classes scenarios, it will may cause class leak

I see it could be just remain weakKeys, softValues will cause that fury must create again when after gc.

@chaokunyang
Copy link
Collaborator

If we remove it, those classes will never be collected. In some hot-loading classes scenarios, it will may cause class leak

I see it could be just remain weakKeys, softValues will cause that fury must create again when after gc.

The values can have strong references to keys. If value are not soft references, the weak key won't make the classes eligible for gc. If we remove soft values. The weak keys should be removed too

@MrChang0
Copy link
Contributor Author

If we remove it, those classes will never be collected. In some hot-loading classes scenarios, it will may cause class leak

I see it could be just remain weakKeys, softValues will cause that fury must create again when after gc.

The values can have strong references to keys. If value are not soft references, the weak key won't make the classes eligible for gc. If we remove soft values. The weak keys should be removed too

I'd like think whether softValue or not will not effect weakKeys. weakKeys will take key reference as WeakEntry, and won't effected be valueReference. maybe we can take a test for it.

@chaokunyang
Copy link
Collaborator

The value may have strong reference to key, so the key are always strong reachable, and it' will never be taken as weak. The thing here is that Fury will hold strong reference to classes of user objects.

@MrChang0
Copy link
Contributor Author

I see

@chaokunyang
Copy link
Collaborator

How about removing weakkeys & soft values, we can make org.apache.fury.util.LoaderBinding#clearClassLoader as a method in ThreadSafeFury, and let users to clear classloader explicitly. Then we can remove all weak/soft values in Fury.

The weak/soft values in CodeGenerator should still be preserved, since it's a static global state

@MrChang0
Copy link
Contributor Author

It is a simple way and users mostly time need't to take this method.
if for may way, I'd like clear Fury#ClassLoader, set ClassLoader when gerFury() and clear classLoader when returnFury(). :)

@chaokunyang
Copy link
Collaborator

It is a simple way and users mostly time need't to take this method. if for may way, I'd like clear Fury#ClassLoader, set ClassLoader when gerFury() and clear classLoader when returnFury(). :)

The thing is that Fury itself doesn't support change classloader currently, so we use a map to switch classloaders. Different classloader may have class with same name but different fields. So Fury made the classloader inmutable for simplicity.

But in the long run, it would be great if we can allow update classloader on Fury itself without introducing extra performance overhead.

chaokunyang added a commit that referenced this issue May 20, 2024
## What does this PR do?

remove soft/weak ref values from thread safe fury

## Related issues

Closes #1632


## Does this PR introduce any user-facing change?

<!--
If any user-facing interface changes, please [open an
issue](https://github.com/apache/incubator-fury/issues/new/choose)
describing the need to do so and update the document if necessary.
-->

- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?


## Benchmark

<!--
When the PR has an impact on performance (if you don't know whether the
PR will have an impact on performance, you can submit the PR first, and
if it will have impact on performance, the code reviewer will explain
it), be sure to attach a benchmark data here.
-->
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants