spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.5k stars 40.53k forks source link

Provide a more memory-efficient NIO-based file system watcher in DevTools #9882

Open dsu opened 7 years ago

dsu commented 7 years ago

File Watcher thread takes about of 98% of entire application memory.Memory consumption is still growing after application start. It is also impossible to exclude log files from third party libraries that need to write to files under resources directory. It would be nice to be able to exclude files by location or by file extension. Something similar to restart.include/restart.exclude properties.

wilkinsona commented 7 years ago

Sorry, I don't understand your description of the problem.

File Watcher thread takes about of 98% of entire application memory

Roughly speaking, a thread consumes a fixed amount of memory that's determined by the JVM's configured thread stack size. A thread itself shouldn't consume 98% of the available memory. Can you please clarify what you mean here?

It is also impossible to exclude log files from third party libraries that need to write to files under resources directory.

Why would a library need to write its output to a location alongside your application's resources?

Before we can assess the need for an enhancement, we'll need to have a better understanding of the problem that you're facing and why it's occurring. To help with that, can you please provide a small sample that we can clone or unzip that reproduces the problem?

dsu commented 7 years ago

It just takes more memory than it should - heapdump. I would like to reduce the number of files that are watched. Probably a few thousands of files under resources directory can help to reproduce the problem.

Why would a library need to write its output to a location alongside your application's resources?

All file paths rely on some configuration property. Library stores all the files and logs under a common root directory. Some of this files need to be accessible as a resources. This is just an example. What does it matter?

wilkinsona commented 7 years ago

Thanks.

What does it matter?

It matters because we can't make a change or enhancement without first fully understanding the problem that we're trying to solve. On that note, as I said above, can you please provide a small sample that we can clone or unzip that reproduces the problem?

dsu commented 7 years ago

Imagine an app that needs to generate thousands of images that needs to be accessible by a reverse proxy..

package com;

import java.io.BufferedWriter;
import java.io.File;
import java.io.IOException;
import java.io.RandomAccessFile;
import java.nio.file.Files;
import java.nio.file.Path;

import javax.annotation.PostConstruct;

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.stereotype.Component;

@SpringBootApplication
public class WatcherKillApp {

    public static void main(String[] args) {
        System.out.println("WatcherKillApp");
        SpringApplication.run(WatcherKillApp.class, args);
    }

    @Component
    public static class FileCloner {

        @PostConstruct
        public void cloneFiles() throws IOException {
            String root = getClass().getResource(".").getFile() + File.separator + ".." + File.separator + "static";
            for (int i = 0; i <= 500000; i++) {
                Path fullPath = new File(new File(root), i + ".jpg").toPath();
                // make sure file exists
                Files.createDirectories(fullPath.getParent());

                try (BufferedWriter bw = Files.newBufferedWriter(fullPath)) {
                    bw.write("test");
                }

                System.out.println("new file " + fullPath.toFile().getAbsolutePath());
            }

        }
    }

}
philwebb commented 7 years ago

I think I see what you're saying. You have an application that has a very large number of static resources. The FileSystemWatcher class stores a map containing FolderSnapshot objects. A FolderSnapshot includes a FileSnapshot for each monitored file (so we know the date/length etc). Since you have so many files, the memory consumption is quite large.

philwebb commented 7 years ago

It's a fairly unusual situation so this isn't a particularly high priority. If anyone wants to take a look a pull-request would be most welcome. In the meantime, you might want to package your static assets into a different JAR so that there's not as much to watch.

fdirlikli commented 6 years ago

Hi @philwebb, Current implementation puts everything in the classpath into memory as FolderSnaphot/FileSnaphot objects and does isRestartRequired checks if there is a change in thoseFolderSnaphot/FileSnaphot objects. This is probably implemented this way because: 1) A single FileSystemWatcher thread is handling both Restart and LiveReload operations 2) LiveReload doesn't have an exclude property (on purpose?) hence it is assumed live reload will require all files to be monitored

But if we introduce the exclude properties for live reload this will give us the possibility to decrease the memory consumption as we can remove the files/folders which are in the intersection set of restart and reload exclusions from the initial snapshots.

Because we removed them from the initial snaphots they are not anymore candidates of ClassPathFileChangeEvents and this will relieve us from firing unnecessary events which has a potential of performance improvement.

I can come up with a PR for this. What do you think about it?

dsu commented 6 years ago

Hello,

I have tried to create exclude properties for that purpose. Currently FileSystemWatcher checks all the files every second. When I add any kind of filtering (compare every filename to a pattern every second ) CPU usage goes very hight and it decreases overall performance greatly.

In my tests java.nio.file.WatchService performs better in terms of memory and CPU consumption but it requires JRE 1.7. Is that a problem?

fdirlikli commented 6 years ago

If we prevent the excluded files from being monitored and put into memory initially we won't need an additional filter for live reload. Any change in a folder or a file that's being monitored will trigger a reload.

philwebb commented 6 years ago

@orwashere It's been quite a while since I've looked at this code but here goes...

Current implementation puts everything in the classpath into memory as FolderSnaphot/FileSnaphot objects and does isRestartRequired checks if there is a change in thoseFolderSnaphot/FileSnaphot objects.

Correct. It monitors all classpath entries that point to folders and creates FolderSnaphot/FileSnaphot for the things it finds.

A single FileSystemWatcher thread is handling both Restart and LiveReload operations

Yeah, sort of. The FileSystemWatcher is pretty limited. It just has the job of monitoring folders and triggering events. The ClassPathFileSystemWatcher makes use of a FileSystemWatcher to monitor the actual classpath and fire ClassPathFileChangeListener.

LiveReload doesn't have an exclude property (on purpose?) hence it is assumed live reload will require all files to be monitored

Not really on purpose, we just didn't consider that someone would want to exclude part of the classpath from monitoring. The ClassPathChangedEvent gets fired by the ClassPathFileChangeListener and it will either trigger a simple web browser refresh or a full application restart. If you look at LiveReloadConfiguration you'll see that optionalLiveReloadServer().triggerReload happens either because we didn't restart the application (we changed css etc) or because we did restart the application and the Spring ApplicationContext was refreshed.

But if we introduce the exclude properties for live reload this will give us the possibility to decrease the memory consumption as we can remove the files/folders which are in the intersection set of restart and reload exclusions from the initial snapshots.

I'd probably say the easiest way to do this would be to offer a higher exclude that provides an opt-out of monitoring. Something like spring.devtools.monitor.exclude. I think that might be slightly clearer than offering spring.devtools.reload.exclude and looking at the intersection.

I can come up with a PR for this. What do you think about it?

If you have time, that would be great!

sothawo commented 6 years ago

is this still an issue after dsu's merge?

wilkinsona commented 6 years ago

@sothawo Nothing was merged into this repository. A GitHub glitch allowed a merge in su's fork to close this issue. The issue was then re-opened. It remains open today, so, yes, it's still an issue. There's an outstanding pull request (#11125) from dsu that we hope to merge before 2.1 is released.

wilkinsona commented 5 years ago

We have decided not to merge the proposed changes. A NIO-based file system watcher is still of interest, but some design work is necessary before we can proceed with an implementation.

xak2000 commented 5 years ago

I just want to add that the issue is not only with high memory usage, but (in my case) with high CPU usage. On Windows 7 x64 File Watcher thread consistently uses 5-10% of 4-core CPU.

The classpath contains around 1000 files (resources (js+css+ftl) + classes).

The application is just doing nothing, but consumes so many CPU resources because of devtools. Constantly. If I run several applications like this they will consume all CPU resources of 4-core i5 CPU. :) This is especially significant with microservice architecture (when developer is required to run multiple applications at once), which spring-boot promotes.

So it is definitely will be valuable to implement some event-based watching, if possible.

wilkinsona commented 4 years ago

Via @odrotbohm, https://github.com/gmethvin/directory-watcher may be of interest here too.