Fallback mechanism for BuildContextHolder.setValue() is slow

[muffl0n]

While debugging hotspots in the plugin I came across the method BuildContextHolder.setValue(). This is used to persist the generated hash codes and uses nearly 20% of the execution time.

The thing that bugged me: There is a fallbackStorage that is used when buildContext is not available. But instead of determining if there is a buildContext once when the class is created, it is checked on every call of setValue(). There is even a fallback in getValue() if the buildContext doesn't exist. Or does not contain the value which can't really happen when the buildContext exists.

But the biggest hit in performance seems to be the persisting of the fallbackStorage. This is done every time setValue() is called.

What is the reason that there is a fallbackStorage at first? What are the chances that there will be no buildContext?

[muffl0n]

I tried my best in proposing a fix. See #197

[alexo]

If I remember correctly, the default build context is not available in older versions of maven.

[alexo]

@muffl0n thanks for looking into that.

[muffl0n]

I don't know why but the "DefaultBuildContext" in plexus-build-api-0.0.7 has useless implementations of getValue() and setValue():

  public Object getValue(String key) {
    return null;
  }

  public void setValue(String key, Object value) {
  }

This way my optimation killes the "incrementalBuild"-feature. :(

[alexo]

@muffl0n I think it would be better to persist to disk asynchronously, so the execution wouldn't be slowed down by this expensive operation.

[muffl0n]

Persisting the storage should be done once in destroy(). Currently destroy() is not being called. But imho this is the best place. Somehow destroy() has to be called at the end of the execution by the Mojo.

Btw, what if someone uses parallelProcessing? I found that there is no synchronisiation in BuildContextHolder for fallbackStorage. Might this be a problem here?

[alexo]

The destroy method should not be called at the end of the execution mojo, since it would break the incremental build feature. Once the storage is destroyed, there is no info about the change in subsequent build execution.

Regarding the synchronization of BuildContextHolder, I have not tested but it probably should be synchronized.

[muffl0n]

destroy() would have to be completely changed, that's true. The fallbackStorage should not be deleted but saved to disk. For example:

public void destroy() {
  OutputStream os = null;
  try {
    os = new FileOutputStream(getFallbackStorageFile());
    fallbackStorage.store(os, "Generated");
    LOG.debug("fallback storage updated");
  } catch (final IOException e) {
    LOG.warn("Cannot safe fallbackStorage to {}, because {}.", getFallbackStorageFile(), e.getMessage());
  } finally {
    IOUtils.closeQuietly(os);
  }
}

[alexo]

Probably it would worth creating another method (ex: persist()) which would explicitly store to disk. The persist() would be invoked after each build execution.

[muffl0n]

I opened a new PR #198 that implements said persist() which is called once at the very end of the plugin's execution. First run on my project: 1m37s Second run: 40s Old value was around: 2m30s

[muffl0n]

Fixed by #198.