5
\$\begingroup\$

I would like to optimize the way that I save files.

Each second I save lots of strings in different files. What I want to do is bunch up these IO tasks so they happen in bursts, instead of spread out randomly.

I want to write files, but without performing too many IO operations at the same time, so I don't put the disk at 100% of I/O-used.
(I'm specially using a Raspberry/HDD).

Currently, I made this code :

public class FileSaverTimer implements Runnable {

    public static final int MAX_RUNNING = 10, SKIP_WHEN_ALREADY = 2;
    private static List<FileSaverAction> allActions = new ArrayList<>();
    public static void addAction(FileSaverAction action) {
        allActions.add(action);
    }
    
    private int actionRunning = 0;
    
    @Override
    public void run() {
        if(actionRunning > SKIP_WHEN_ALREADY)
            return; // too many already running, skipping...
        for(int i = actionRunning; i < Math.min(MAX_RUNNING, allActions.size()); i++) {
            FileSaverAction action = allActions.remove(0); // removing first item
            actionRunning++; // adding a running task
            action.save(() -> actionRunning--); // save, and when it's finished removing running task
        }
    }

    public static interface FileSaverAction {
        
        /**
         * Technically save the save sync with the thread which run this
         * 
         * @param finished the action to run when it's finished
         */
        void save(FileSaverFinished finished);
    }

    public static interface FileSaverFinished {
        
        /**
         * The action that will be called when the action is finished.
         */
        void finish();
    }
}

Then, to use:

FileSaverTimer.addAction((finished) -> {
    try {
        Files.write(file, content.getBytes(), StandardOpenOption.APPEND);
    } catch (IOException e) {
        e.printStackTrace();
    }
    finished.finish();
});

There is only one instance of the timer which runs asynchronously each second; it's called like this:

Bukkit.getScheduler().runTaskTimerAsynchronously(this, new FileSaverTimer(), 20, 20);
// 20 ticks = 1 second

The objective is to add files in a queue, and wait until previous tasks are done. But I don't know how I should define limits SKIP_WHEN_ALREADY and MAX_RUNNING, or if it's really relevant.

Some informations :

  • This code works. (I'm sure I can do better)
  • Some files can be back in the queue quickly with other information. (I can know which are more likely to come back.)
  • It's not necessary to save stuff as soon as possible. (Everything stays in RAM.)

So, can you help me to do better? Specially to work better according to user hardware?

Is it better to make lots of little file saves (just a few lines each), or only one big save?

Also, I want to know if you suggest another structure. I just proposed my code as a baseline!

\$\endgroup\$
6
  • 2
    \$\begingroup\$ You're using a static, non thread safe collection across all instances of your saver class, however the actionsRunning is non-static. Are you expecting access from multiple threads/instances? How are instances of the timer being created/what is invoking the run method? \$\endgroup\$
    – forsvarir
    Oct 20 '21 at 19:24
  • \$\begingroup\$ There is only one instance of the timer that is running, it's static to make is faster to register file to save. They all will be used only by one instance of this thread \$\endgroup\$
    – Elikill58
    Oct 20 '21 at 19:33
  • 1
    \$\begingroup\$ Can you share a bit more details about what kind of optimization are you looking for? Have you identified problems with your current approach, for example bottlenecks, or low throughput, or others. How would you measure the success of optimizations? \$\endgroup\$
    – janos
    Oct 21 '21 at 14:56
  • 1
    \$\begingroup\$ Welcome to Code Review! "without making the disk at 100%" - is that 100% used space? or something else? \$\endgroup\$ Oct 21 '21 at 16:48
  • 1
    \$\begingroup\$ No, 100% of used performance, not space. Not taking all power of disk and so impact other files/apps. But thanks :) \$\endgroup\$
    – Elikill58
    Oct 21 '21 at 16:51
4
+50
\$\begingroup\$

I'm probably not polished enough with Java to propose new code, so here are some abstract thoughts anyone else can use:

  • Fewer bigger writes will generally be more efficient. Disk access is heavily optimized at the OS level, and those optimizations will work best if the number of things you're doing (from the disk's perspective) is smaller.
  • It seems likely that you have more abstraction here than you need. Do you expect to need the lambdas/inversion-of-control pattern you're using? The less fancy approach would be to make FileSaverAction an actual class. Its constructor would take in the file and content values, its save method would take in the FileSaverTimer who's actionRunning needs to be decremented. This may be a little more verbose, but I think it will help with other optimizations.
  • Reading and writing to disk is often heavy, but opening and closing files is also heavy. It sounds like you expect many of these writes to be to the same files; consider batching them. I think roughly how that would work is that you'd replace the List allActions with some kind of mapping (a default-dict?) from files to lists of contentss. To handle an item, you'd open the file, do all of the writes, and then close it. Depending what the distribution of expected incoming file-write tasks is like, you could even maintain a pool of open file handles.
  • Check the buffering settings on the file-manipulation methods you use. Exact tuning is probably hard, but it's worth checking if the defaults are sane for your situation.

Some less abstract stuff:

  • allActions can and probably should be final. (The list can change, but it will always be the same list.)
  • Since you're relying on there only ever being one FileSaverTimer, you should explicitly use the Singleton pattern.
  • The for loop looks sketchy as written. Probably something like this would be better:
for(int i = actionRunning; allActions.hasNext() && i < MAX_RUNNING); i++) {

Actually, as I think more about MAX_RUNNING and SKIP_WHEN_ALREADY, the less I like them.
As I understand, you need MAX_RUNNING because you want these bursts of IO to not be too big. (It's still not clear that this should matter, but that's on you.) You need SKIP_WHEN_ALREADY because, if sessions of run are taking longer than a second, you don't want them overlapping too much.
But any overlap is probably "too much"! At best, multiple sessions of run acting on allActions and actionRunning at the same time will be redundant, but depending how runTaskTimerAsynchronously works, you could have really bad concurrency issues.
A better approach would probably be to check if there's an active session of run, and if there is then somehow increase its "burst budget" and abort the new run.

One subject I haven't gotten into, because I don't know if it'll work for your context, is parallelization of the IO tasks. If multi-threading is an option, that would probably let you get the same amount of IO work done in a shorter denser burst. But I don't know if that would be good for you, and it'll be an entirely different design than what you're working on now.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for your answer. It's a good idea to remove the abstract class which isn't very usefull. Also, about file handle, do you suggest to keep them for long time, or close them like each minutes to don't overflow with too many files handle ? For you final question, I don't want to make lot of thread for those type of I/O because they are not very important one. I prefer l'ose tome than make lot of thread (and my project already use lot of thread) \$\endgroup\$
    – Elikill58
    Nov 28 '21 at 15:55
  • \$\begingroup\$ Having too many files open at once is a problem (what problems does it cause? I don't know. Probably just bad performance). How many is too many? I don't know. Keeping any single file handle (that you still expect to use) open for a long time probably isn't a problem, except that you can't assume the data is "really there" until it's closed. \$\endgroup\$ Nov 28 '21 at 23:51
  • \$\begingroup\$ If you determined that recycling open file handles will be advantageous, and you figure out some number you don't want to exceed, a simple strategy (from cache theory) that would probably work would be: Have a pool of open connections. Don't close connections after you use them. When you need to open a file that's not already in the pool (and the pool has reached its max size), close the least-recently-used connection in the pool to make room. Someone else can point out all the problems with this strategy :) \$\endgroup\$ Nov 28 '21 at 23:56
  • \$\begingroup\$ Ok thanks for all informations ! Also don't worry, I don't forget for check & bounty, I think I will apply them tomorrow (after testing what you said) \$\endgroup\$
    – Elikill58
    Nov 29 '21 at 8:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.