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

The Cache used by ArchUnitRunner should allow some option to be cleaned after the test run #45

Closed
codecholeric opened this issue Nov 16, 2017 · 8 comments
Assignees
Milestone

Comments

@codecholeric
Copy link
Collaborator

At the moment imported classes for a specific set of URLs will stay in memory forever. While this might be useful, if multiple tests in fact want to import the same set of classes, it might be necessary in some scenarios, to empty the cache after each test class is finished.
Proposal: It should be possible to declare

@AnalyzeClasses(cache = CacheMode.FOR_CLASS)

where the default is the current behavior CacheMode.FOREVER.

@codecholeric
Copy link
Collaborator Author

I've reconsidered this, because I think the desired behavior should be pretty much the same for everyone: "If you have the memory, just cache the classes, if you run out of memory, clear them".

So I think, it's better to just make the cache smarter, which should be achievable by using the correct references:

  • The cache by class can use strong references, if the runner clears them after the test run. This makes sense, since the scope is clearly defined, while the test class is running, we want to keep the classes, once the test run is over, we want to discard them, with respect to the class as key.
  • The cache by locations should use soft references, i.e. hold those references as long as possible, but drop them, once the heap limit is reached. This will enable multiple ArchTests to reuse the imported classes for the same locations, but still prevent successive tests from failing due to low memory.

I did some experiments and with soft references (as opposed to weak references, where the references are dropped at will, even if there is sufficient memory, until the same classes have been imported several times) the behavior seems correct. The Cache might trigger an import for every test, if the heap is small, but only the minimally necessary time, if the heap is big enough. I don't know, how the behavior would be, if the heap is already pretty filled with other dangling objects (i.e. what will be cleared first), but this should be a fair compromise between usability and performance.

Also as far as I can see (happy to hear objections 😉), this would also resolve #44 .

@thmuch
Copy link
Contributor

thmuch commented Nov 27, 2017

I can confirm the cache is cleared if there is not enough heap available for the tests that run after ArchUnit!

But: We still see a noticeably slowdown in our build, because the soft references don't seem to be cleared quick enough.

I'm not sure if ArchUnit should be adapted to our special use case any further as the soft ref cache is working. We're back to ArchUnit 0.4 and the old cache clear workaround for the time being. Maybe I can use importClasspath() in BeforeClass (instead of AnalyzeClasses) and run the tests in JUnit methods (instead of using ArchTest fields).

@codecholeric
Copy link
Collaborator Author

Oh, I'm sorry to hear that, I hoped, the issue would be gone with the soft references. I don't think, this is desirable behavior of the cache though, to noticeably slow down consecutive test executions 😉
I'll reopen this issue and look into it, as soon as I have time.
If you have only one test, you could of course import the classes manually in @BeforeClass and throw them away in @AfterClass. It might be better to importPackages(..) though, instead of the whole importClassPath() (since that brings a lot of 'useless' classes, from test infrastructure to the whole JDK classes)
I'll still try to find a solution, in the worst case it's back to an additional configuration to override the caching behavior, for example to completely skip the caching by locations and only cache for the test run (that would be a trivial change and IMHO a valid use case).

@codecholeric codecholeric reopened this Nov 27, 2017
@thmuch
Copy link
Contributor

thmuch commented Nov 28, 2017

To put this into context: We're running ArchUnit first in a suite of ca. 7500 tests. Ca. 500 of these tests use HSQL, and the first few of these HSQL based tests are slowed down (the slowdown is roughly 1 minute in a total test time of 8 minutes). After the soft refs have been cleared, all tests run with their usual speed.
So, I think your cache is working for most scenarios. After all, the soft refs get cleared and all tests are run successfully.
We've got plenty of options to use the current release of ArchUnit (use importPackages, tune our environment or project setup etc.) It's just we can't use AnalyzeClasses in our setup, but I'm happy with the alternatives.

@codecholeric
Copy link
Collaborator Author

You did try @AnalyzeClasses(packages = ...) though, right? 😉 (since you make it sound, like it's @AnalyzeClasses OR importPackages)
Also there is @AnalyzeClasses(importOptions = ...) where you can fine-tune the URLs to import (you can even implement your own ImportOption, where you can decide exactly about every single URL, if you want to import the class)
But one way or the other, if there are serious drawbacks from the cache, like a noticeable slowdown for consecutive tests, I'll add an option to cache classes only for the test run, if I can't find a better solution.
I would guess, that the GC keeps the soft references up to the last moment, and then suddenly has to perform so much clean up in a single shot, that it has a serious performance impact ☹️

@codecholeric codecholeric modified the milestones: Release 0.5.0, 0.6.0 Dec 4, 2017
@codecholeric codecholeric modified the milestones: 0.6.0, 1.0.0 Feb 23, 2018
@codecholeric codecholeric modified the milestones: 1.0.0, 0.6.0, 0.8.0 May 16, 2018
codecholeric added a commit that referenced this issue May 16, 2018
…R_CLASS completely deactivates the cache by locations.

Resolves: #45
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
@athkalia
Copy link

Hi @codecholeric , I am experiencing some out of memory issues myself when running ArchUnit Tests. Do you have any best practices to suggest overall?

@codecholeric
Copy link
Collaborator Author

@athkalia it's hard to tell without any background. Since you replied to this issue, do you think it's cache related? You should not run out of memory though, since the cache uses soft references.
If you have a lot of classes you might have to increase the heap, there is no way around it. I've had cases where importing 50000 classes would need around 6GB of heap to run arch rules.
So maybe to better help you:

  • how many classes are you trying to import?
  • how much heap did you configure?
  • is there a difference between using @ArchTest style and a plain classFileImporter.import...(..)?
  • are you running out of memory during the import or while checking rules?

@athkalia
Copy link

HI @codecholeric, I think your answer answers my question. It's a very big project and after adding ArchUnit tests I had to increase the heap size from about 400MB to 2GB and that seemed like a lot. But if you have experienced that and even more then I rest my case. I am importing around 15,000 classes, and that is with classFileImporter.import...(..).

codecholeric added a commit that referenced this issue Feb 21, 2021
…erences for the classes cached by location. cached by test still uses hard references, but is explicitly cleared by the runner after the test class is done. Also improved ProGuardTest, to consider transitive Guava dependencies. Unfortunately that also means, the artifact will be a lot bigger in the end, since more classes need to be retained uncrippled to ensure archunit-junit has all necessary dependencies. This should make it unnecessary, to obtain explicit control over the cache behavior as initially demanded in #45. Instead we now rely on the GC to clean up the imported classes, if the heap runs low.

Resolves #45
codecholeric added a commit that referenced this issue Feb 21, 2021
…R_CLASS completely deactivates the cache by locations.

Resolves: #45
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
hankem added a commit that referenced this issue Nov 13, 2022
Bumps \[gradle/wrapper-validation-action\](https://github.com/gradle/wrapper-validation-action) from 1.0.4 to 1.0.5.

# Release notes

from [gradle/wrapper-validation-action's releases](https://github.com/gradle/wrapper-validation-action/releases):

> ## v1.0.5
> ------
> 
> ### Gradle Wrapper Validation
> 
> *   Update dependencies for Node 16 ([#53](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/53))
> *   Update dependencies with security vulnerabilities ([#67](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/67))
> *   Update various other dependencies ([#45](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/45), [#47](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/47), [#48](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/48), [#54](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/54))

# Commits

*   [`55e685c`](gradle/wrapper-validation-action@55e685c) Dependencies
*   [`f232d2e`](gradle/wrapper-validation-action@f232d2e) Merge branch 'master' into releases/v1
*   [`9aa31f2`](gradle/wrapper-validation-action@9aa31f2) Merge pull request [#67](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/67) from gradle/dd/update-deps
*   [`5d6ea91`](gradle/wrapper-validation-action@5d6ea91) Extend timeout for Jest tests
*   [`db7df1f`](gradle/wrapper-validation-action@db7df1f) Update dependencies with vulnerabilities
*   [`859c332`](gradle/wrapper-validation-action@859c332) Merge pull request [#53](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/53) from KengoTODA/node-16
*   [`d0ffc95`](gradle/wrapper-validation-action@d0ffc95) ci: install node v16 instead of v12
*   [`6793660`](gradle/wrapper-validation-action@6793660) Merge remote-tracking branch 'upstream/master' into node-16
*   [`781fa15`](gradle/wrapper-validation-action@781fa15) Merge pull request [#54](https://github-redirect.dependabot.com/gradle/wrapper-validation-action/issues/54) from gradle/dependabot/npm\_and\_yarn/ansi-regex-5.0.1
*   [`7606dd0`](gradle/wrapper-validation-action@7606dd0) Bump ansi-regex from 5.0.0 to 5.0.1

Additional commits viewable in [compare view](gradle/wrapper-validation-action@v1.0.4...v1.0.5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants