felix icon indicating copy to clipboard operation
felix copied to clipboard

Do not attempt to process files when there are none

Open set321go opened this issue 9 years ago • 3 comments

I tried to create a ticket for this but I can't create tickets in ASF jira.

Fileinstall was surprisingly at the top of the hotspot list while profiling today while running fileinstall 3.5.0 after upgrading to 3.5.4 there were some improvements. Even after stretching the polling from the default 2sec to 60sec it still used a lot of cpu. My file install directory contains 4 config files and no subdirectories.

This change prevents process being called when files is empty which is most of the time.

I also noticed locally that during a poll this line of code was called multiple times (usually 4).

set321go avatar May 13 '16 18:05 set321go

This is a semantics change, as previously the call would cause failures to be re-tried. should failures be empty and files be empty the code would be actually fast.

as I have profiled fileinstall and was responsible for some of the improvements in 3.5.4, i would like to see the profile, maybe i am missing something. in our case the code is no longer hot and we poll every second hundreds of files.

CodingFabian avatar May 13 '16 19:05 CodingFabian

I'm not sure what you mean. Looking at Scanner.scan it news up a TreeSet as the return type, the only way i'm getting anything other than a Set is an Exception which scan does not catch, tbh you could probably remove the null check entirely. https://github.com/set321go/felix/blob/7a6bdb0895674f2359a2a05ee50f802baaa612c2/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/Scanner.java#L131

I can get some of the profiling data next week, I don't have it here.

set321go avatar May 14 '16 00:05 set321go

@set321go out of coincidence my profiling arrived now also here as optimization candidate, so I had a look at it. What i meant is that doProcess will not only work on files but also on processingFailures. So while there would be no changes on file system, the mechanism would retry the processing failures.

so a change I would approve is

                     if (!files.isEmpty() || !processingFailures.isEmpty()) {
                          process(files);
                      }

which would not do much, but avoid locking.

CodingFabian avatar Jun 24 '16 07:06 CodingFabian