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

feat(java): remove soft/weak ref values from thread safe fury #1639

Merged
merged 1 commit into from
May 20, 2024

Conversation

chaokunyang
Copy link
Collaborator

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?

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

@chaokunyang
Copy link
Collaborator Author

chaokunyang commented May 16, 2024

Hi @MrChang0 , could you help review this PR. I plan to make a new release for fury when this got merged. We have some minor bugs in fury 0.5.0 release

@MrChang0
Copy link
Contributor

great, I thinking about that we can add classLoaderFuryPooledCache.cleanup() in getFury(), make cache clean more timely.

@chaokunyang
Copy link
Collaborator Author

chaokunyang commented May 17, 2024

cleanup

@MrChang0 Could you elaborate more details? If we cleanup classloader in getFury, Fury will be created every time when getFury is called

@MrChang0
Copy link
Contributor

we call cleanup() to clean data which is expired or drain key/value reference. because cache will hold on data much more time even data is needed to be clean. so if the cache does't operate frequently enough, we can use cleanup(). maybe it can solve class leak question.

@chaokunyang
Copy link
Collaborator Author

we call cleanup() to clean data which is expired or drain key/value reference. because cache will hold on data much more time even data is needed to be clean. so if the cache does't operate frequently enough, we can use cleanup(). maybe it can solve class leak question.

I still don't get it. We cleanup the classloader and associated map in:

  public void clearClassLoader(ClassLoader loader) {
    classLoaderFuryPooledCache.invalidate(loader);
    classLoaderLocal.remove();
  }

Could you share some code?

@MrChang0
Copy link
Contributor

just like

  @Test
    public void testCache() throws InterruptedException {
      Cache<String, String> cache = CacheBuilder.newBuilder().expireAfterAccess(Duration.ofSeconds(1)).build();
      cache.put("1", "1");
      cache.put("2", "2");
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
      cache.cleanUp();
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
      Assert.assertEquals(cache.size(), 2);
      cache.cleanUp();
      Assert.assertEquals(cache.size(), 1);
  }

@chaokunyang
Copy link
Collaborator Author

just like

  @Test
    public void testCache() throws InterruptedException {
      Cache<String, String> cache = CacheBuilder.newBuilder().expireAfterAccess(Duration.ofSeconds(1)).build();
      cache.put("1", "1");
      cache.put("2", "2");
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
      cache.cleanUp();
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
      Assert.assertEquals(cache.size(), 2);
      cache.cleanUp();
      Assert.assertEquals(cache.size(), 1);
  }

Clean it up every time will create Fury everytime, which is very expensive. Fury needs be cached.

@MrChang0
Copy link
Contributor

MrChang0 commented May 19, 2024

I explain it.

  @Test
    public void testCache() throws InterruptedException {
      Cache<String, String> cache = CacheBuilder.newBuilder().expireAfterAccess(Duration.ofSeconds(1)).build();
      cache.put("1", "1");
      cache.put("2", "2");
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
      // clean up do nothing, because key1, key2 both alive.
      cache.cleanUp();
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
      Assert.assertEquals(cache.size(), 2);
      Thread.sleep(800);
      cache.getIfPresent("1");
     // key2 is expired, but won't be cleared from cache. may be case memory leak. now cache size is 2.
      Assert.assertEquals(cache.size(), 2);
      cache.cleanUp();
     // clean up clear key2, because key2 is expired. so size of cache becomes 1.
      Assert.assertEquals(cache.size(), 1);
  }

I mean that when cache have expireAfterAccess option. it may save data which should expired too much time.
so we cleanup timely won't create Fury everytime, it just clear data which is expired.

@chaokunyang
Copy link
Collaborator Author

I see, thanks. Clean up it every time is not cheap for small objects.
image
image

Fury will replace guava cache with a fury implemented one, see more on #1113 . We can support clean up cache timely when access expired

@chaokunyang chaokunyang merged commit 03dba11 into apache:main May 20, 2024
32 checks passed
@MrChang0
Copy link
Contributor

fine, if we want remove guava dependency, we can try caffeine, maybe can shaded in fury.

@chaokunyang
Copy link
Collaborator Author

Yep, this is a smaller dependency. Fury is a low-level library, we want to make a zero-dependency in the long time. The cache usage in Fury is very simple. Maybe we can implement it manually too.

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.

[java] FuryPooledObjectFactory#classLoaderFuryPooledCache remove softValues
3 participants