[PATCH] D17066: libLTO: add a ThinLTOCodeGenerator on the model of LTOCodeGenerator.

[Teresa Johnson via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

Thanks for adding the description! I do think that the formula for the new cache size is not right, see suggested fix below.

A couple misc comment typos, but LGTM once these the above is addressed.


================
Comment at: include/llvm-c/lto.h:687
@@ +686,3 @@
+/**
+ * Sets the maximum cache size that can be persistent across build, in term of
+ * percentage of the available space on the the disk. Set to 100 to indicate
----------------
s/term/terms/

================
Comment at: include/llvm-c/lto.h:690
@@ +689,3 @@
+ * no limit, 50 to indicate that the cache size will not be left over the
+ * available space. A value over 100 will be reduced to 100.
+ *
----------------
"left over half the available space"? (i.e. add "half")  Either that or "left over the free space" (since AvailableSpace = FreeSpace + CacheSize)?

================
Comment at: include/llvm-c/lto.h:694
@@ +693,3 @@
+ *  AvailableSpace = FreeSpace + ExistingCacheSize
+ *  NewCacheSize = (100*P)/AvailableSpace
+ *
----------------
This doesn't seem right. I think P should be divided by 100 and not multiplied by it, since it is a percentage. And I think the percentage needs to be multiplied by something, not divided by AvailableSpace?

Should this be: 
NewCacheSize = AvailableSpace * P/100

since it is described as the percentage of available space used for the cache.

For this and the above suggestion, any change needs to be replicated below in ThinLTOCodeGenerator.h.

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:104
@@ +103,3 @@
+   *  - The pruning expiration time indicates to the garbage collector how old
+   *an
+   *    entry needs to be to be removed.
----------------
move 'an' to next line.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:66
@@ +65,3 @@
+  } else {
+    ModuleOrErr = parseBitcodeFile(Buffer, Context);
+  }
----------------
joker.eph wrote:
> tejohnson wrote:
> > The when lazy metadata linking is enabled, metadata parsing should be postponed until we actually go to do the import and linkInModule during the bulk importing. Since the source module is destroyed at the end of linkInModule, this should reduce the max amount of live metadata to one module at a time, rather than all modules we are importing from. I'm surprised it didn't reduce the max memory usage in a debug build.
> > 
> > If we ever go back to post-pass metadata linking in the function importer (i.e. if we decide to do iterative importing rather than a single bulk import from each module), this will become more critical. However, if we move to summary-only importing decisions it will obviate the need to do this.
> > 
> > Until we go to summary only import decisions, I would think it should reduce the max memory usage as per my first paragraph. Did you see a cost going with lazy metadata loading?
> So just tested:
> 
> With lazy loading of metadata:       getLazyBitcodeModule takes 237ms and materializeMetadata takes 74ms (total 314ms)
> without lazy loading of metadata:  getLazyBitcodeModule takes 316ms 
> 
> So no perf diff.
> 
> I probably don't see any diff on the memory because most metadata will leak to the context and being free'd with the Module. So there is no real impact on the peak memory (just delaying a little bit).
Ok, thanks for checking!


http://reviews.llvm.org/D17066