1
\$\begingroup\$

I am trying to improve the performance of one of my project's methods, which obtains the command line of a process using WMI. A summary of key points:

  • The method is on an object representing a Windows Process.
  • Many (hundreds) of such objects will exist
  • The code to obtain the command line for a single process takes ~50 ms to run.
  • The code to obtain the command lines for all processes takes ~200 ms to run.
  • The goal of my code is to pre-fetch the results for all processes and store in a map, so other objects can simply fetch their results from the map
  • If I get all ~200 processes one by one, the whole iteration takes over 10 seconds. If I fetch once and cache, the entire iteration takes only ~250ms.
  • The map key is a process ID. These can be re-used but the new process will have a later start time than the time the original key was inserted.
  • To keep the map from retaining too much stale data, I desire to periodically clear it. Iterating over stale elements is much less efficient than a clear-and-refill.

In theory, it wouldn't hurt for multiple threads to be storing information at the same time, as they would be writing the same key and value. However, the cost of locking is probably much lower than the cost of fetching the information to store, so one key goal is to only have a single thread executing the expensive call.

I considered the use of ConcurrentHashMap but think with the way my get and put are sequenced that it wouldn't provide sufficient checking.

Questions I have:

  • Is this code thread safe?
  • If not, can it be made so?
  • Could using a ReentrantReadWriteLock improve this?
  • I continually see "static maps are a bad idea" but don't see a good alternative here. Is there an obvious (to others), safer option?

I have attempted to extract the relevant portions of the code with the map locking, unlocking, and access below which should be sufficient for review. The full code can be accessed on my PR here.

public class WindowsOSProcess extends AbstractOSProcess {

    // The map and its lock. Static for intended access by all process objects
    private static final Map<Integer, Pair<Long, String>> commandLineCache = new HashMap<>();
    private static final ReentrantLock commandLineCacheLock = new ReentrantLock();

    // Each object will only fetch its own string once via this memoized getter
    private Supplier<String> commandLine = memoize(this::queryCommandLine);

    // The public facing API method that fetches via the memoizer
    @Override
    public String getCommandLine() {
        return this.commandLine.get();
    }

    // The query method. Could be called from multiple different
    // objects at the same time
    private String queryCommandLine() {
        commandLineCacheLock.lock();
        Pair<Long, String> pair = commandLineCache.get(getProcessID());
        // Valid process must have been started before map insertion
        if (pair != null && getStartTime() < pair.getA()) {
            // Entry is valid, return it!
            commandLineCacheLock.unlock();
            return pair.getB();
        } else {
            // Invalid entry, rebuild cache
            // Invalidate processes started after this time
            long now = System.currentTimeMillis(); 
            // "Expensive" method, takes ~200ms
            WmiResult<CommandLineProperty> commandLineAllProcs = Win32Process.queryCommandLines(null);
            // Periodically clear cache to recover resources when its size is 2*# of
            // processes
            if (commandLineCache.size() >= commandLineAllProcs.getResultCount() * 2) {
                commandLineCache.clear();
            }
            // Iterate results and put in map, storing current PID along the way
            String result = "";
            for (int i = 0; i < commandLineAllProcs.getResultCount(); i++) {
                int pid = WmiUtil.getUint32(commandLineAllProcs, CommandLineProperty.PROCESSID, i);
                String cl = WmiUtil.getString(commandLineAllProcs, CommandLineProperty.COMMANDLINE, i);
                commandLineCache.put(pid, new Pair<>(now, cl));
                if (pid == getProcessID()) {
                    result = cl;
                }
            }
            commandLineCacheLock.unlock();
            return result;
        }
    }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

One thing that immediately jumps out at me is, that you don't use a finally-block to free the lock (as is recommended in the according javadoc btw.), i.e. if there's a single exception in processing, your whole code will be basically dead, as the lock will never be freed anymore.

Thus, to cite the docs, I'd rather do:

commandLineCacheLock.lock();
try {
    // do all the processing here
}
finally {
    commandLineCacheLock.unlock();
}
\$\endgroup\$
4
  • \$\begingroup\$ Thanks! And now that I see I'm basically just wrapping the whole code block I'm wondering if I shouldn't just use synchronized (on a static object such as the map itself) \$\endgroup\$ May 6, 2020 at 6:02
  • \$\begingroup\$ As synchronized carries a bad gut-feeling for me (probably just subjective) I'd stick to the lock. Apart from that, you might reallly benefit from a ReadWrite-lock, but sorry, at this time I just really cannot wrap my head around this. (Just got up ;-)) \$\endgroup\$
    – mtj
    May 6, 2020 at 6:07
  • \$\begingroup\$ I considered a double-checked implementation where the first map read was outside the lock for performance, with a second read inside it... but that's really probably overkill here... \$\endgroup\$ May 6, 2020 at 6:15
  • \$\begingroup\$ I'll go ahead and accept this since it was an extremely valuable contribution. After a night's sleep I think I will keep the same core locking code here but move to a singleton object (using.a memoizer for thread safety of the instance). \$\endgroup\$ May 6, 2020 at 15:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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