Add a sidetable cache in the module for VTable entries, replacing linear lookup #5053

shajrawi · 2016-09-28T21:43:16Z

radar rdar://problem/28311199

This commits resolves a FIXME in SILVTable::getImplementation: Building a sidetable cache in the module for function lookup.

That, in turn, drastically reduces Swift's compilation time in programs with large VTables.

shajrawi · 2016-09-28T21:43:55Z

@swift-ci please smoke test

shajrawi · 2016-09-28T21:44:24Z

@eeckstein Can you please review?

eeckstein

One additional comment:
As the cache is always up-to-date it would be a good idea to add a check in SILVerifier if the cache is consistent with the actual vtable entries. This means:
-) iterate over all vtables
-) check if there is a cache entry for each vtable entry
-) check if the number of total vtable entries == the size of the cache

eeckstein · 2016-09-28T21:53:58Z

include/swift/SIL/SILModule.h

+  /// This is a cache of vtable entries for quick look-up
+  llvm::DenseMap<const SILVTable *, llvm::DenseMap<SILDeclRef, SILFunction *>>
+      VTableEntryCache;
+


llvm::DenseMap<std::pair<const SILVTable *, SILDeclRef>, SILFunction *> would be much more efficient

Actually ... I'm not sure about this. Probably it would be more efficient, but I don't know if this is significant.

Anyway, instead of this outer DenseMap you can directly place the inner DenseMap into SILVTable directly.

So either you do a combined cache in SILModule (DenseMap<std::pair<const SILVTable *, SILDeclRef>, SILFunction *>) or separate caches (DenseMap<SILDeclRef, SILFunction *>) in SILVTable.

Did a combined cache in SILModule (DenseMap<std::pair<const SILVTable *, SILDeclRef>, SILFunction *>), will update the PR after adding a check in SILVerifier

eeckstein · 2016-09-28T21:55:52Z

lib/SIL/SILModule.cpp

+      EntryCache.insert(entry);
+    }
+  }
+}


I think this function is not needed (See below)

Needed unless we want to add friend classes (See below)

eeckstein · 2016-09-28T21:57:43Z

lib/SILOptimizer/IPO/DeadFunctionElimination.cpp

@@ -374,6 +374,7 @@ class DeadFunctionElimination : FunctionLivenessComputation {
        return false;
      });
    }
+    Module->recalculateVTableEntryCache();


Instead of recalculating the whole cache you could just erase the dead entries in the loop above

I can't without making the cache public and/or allow public mutability. either that or make the optimization (and any other future optimizations) friends with the Module

Why not just add SILModule::removeFromVTableCache(SILVTable *, SILDeclRef)

Can do that, will create such a version, my rationale was mitigating the risk of leaving the table in an inconsistent state (By removing an entry from cache but keeping it in the vtable) - but, your solution is more efficient compilation-time wise.

swiftix · 2016-09-28T22:35:39Z

@shajrawi Joe, do we also need the same optimization for witness tables?

shajrawi · 2016-09-28T22:55:00Z

@swiftix As far as I can tell we do not: SIL Modules have DenseMap(s) for witness table lookups, Witness table itself doesn't do any linear lookups and I don't see this as an issue in 'instruments'

shajrawi · 2016-09-28T23:25:55Z

@swift-ci please smoke test

shajrawi · 2016-09-28T23:26:57Z

@eeckstein requested changes / SILVerifier add-on are in amended PR

eeckstein · 2016-09-28T23:55:42Z

lib/SILOptimizer/IPO/DeadFunctionElimination.cpp

        if (!isAlive(entry.second)) {
          DEBUG(llvm::dbgs() << "  erase dead vtable method " <<
                entry.second->getName() << "\n");
+          // Update cache:
+          Module->removeFromVTableCache(&vTable, entry.first);


Wait... it would be cleaner and simpler to remove the cache entry in SILVTable::removeEntries_if. Then you also don't need SILModule:: removeFromVTableCache.

SILVTable doesn't have the module as a class member and passing the module to remove_if would be a problem: the function is templated (typename being the Predicate), implemented inside the header file, the header file has a forward deceleration of the SIL Module (can't include it because SIL Module uses SILVTable)

And if SILVTable::removeEntries_if could have cleanly used the Module I wouldn't have written my previous solution in the first place nor added this public function now: SILVTable is friend class of SILModule :)

I still think it would be cleaner, because all the cache handling is done inside SILVTable.
You can get the module from the SILFunction of the vtable entry (which you are about to delete).
To solve the forward-declaration problem, you can define a private SILVtable helper function, which is defined in the cpp file.

Alright - Done the change you suggested - see amended PR

eeckstein · 2016-09-28T23:57:08Z

lib/SIL/SILVerifier.cpp

  llvm::DenseSet<ClassDecl*> vtableClasses;
+  size_t EntriesSZ = 0;


nitpick: you can move this line down to the loop where it's used.

No: the assert is outside of the loop, you need to sum-up all the entries from all the vtables then compare the overall sum to the cache size.

Sorry, I was confused by the nested loop

shajrawi · 2016-09-29T00:35:25Z

@swift-ci please smoke test

lattner · 2016-09-29T00:45:23Z

Very nice @shajrawi !

gottesmm

Looks good! Some small nits that improve QoI verifier error feedback and style.

gottesmm · 2016-09-29T03:43:42Z

lib/SIL/SILVerifier.cpp

  llvm::DenseSet<ClassDecl*> vtableClasses;
+  size_t EntriesSZ = 0;


nitpick: We generally do not use size_t in these cases. Please use unsigned.

gottesmm · 2016-09-29T03:47:44Z

lib/SIL/SILVerifier.cpp

+    for (auto entry : vt.getEntries()) {
+      assert(VTableEntryCache.find(std::make_pair(&vt, entry.first)) !=
+                 VTableEntryCache.end() &&
+             "VTable entry not in cache");


Can you follow the code above and dump out the entry that is not in the cache?

gottesmm · 2016-09-29T03:48:42Z

lib/SIL/SILVerifier.cpp

@@ -3546,15 +3546,25 @@ void SILModule::verify() const {
    g.verify();
  }

-  // Check all vtables.
+  // Check all vtables and vtable cache.


Grammer nit: "Check all vtables and the vtable cache"

gottesmm · 2016-09-29T03:49:44Z

lib/SIL/SILVTable.cpp

+  // Update the Module's cache with new vtable + vtable entries:
+  for (auto &entry : Entries) {
+    M.VTableEntryCache.insert(
+        std::make_pair(std::make_pair(vt, entry.first), entry.second));


I think you can replace this with:

M.VTableEntryCache.insert({{vt, entry.first}, entry.second});

But my memory might be wrong.

shajrawi · 2016-09-29T17:02:10Z

@swift-ci please smoke test

gottesmm · 2016-09-29T17:58:47Z

Looks good! = D

gottesmm

Looking good. Just one last nit. I am not going to review this again, so no more nits after this. I promise = ).

gottesmm · 2016-09-29T17:59:55Z

lib/SIL/SILVTable.cpp

  return nullptr;
 }

+void SILVTable::removeFromVTableCache(Pair &entry) {
+  SILModule &M = entry.second->getModule();
+  M.VTableEntryCache.erase(std::make_pair(this, entry.first));


If you want, you can also remove the make_pair here and just use {this, entry.first}.

replaced all make_pair(s) with {}

…ear lookup

shajrawi · 2016-09-29T18:08:28Z

@swift-ci please smoke test

eeckstein requested changes Sep 28, 2016

View reviewed changes

shajrawi force-pushed the vtable_entry_cache branch from 4569ffe to 24d5ff6 Compare September 28, 2016 23:24

eeckstein reviewed Sep 28, 2016

View reviewed changes

shajrawi force-pushed the vtable_entry_cache branch from 24d5ff6 to e0f0081 Compare September 29, 2016 00:34

eeckstein approved these changes Sep 29, 2016

View reviewed changes

gottesmm reviewed Sep 29, 2016

View reviewed changes

shajrawi force-pushed the vtable_entry_cache branch from e0f0081 to ccf987e Compare September 29, 2016 17:00

gottesmm reviewed Sep 29, 2016

View reviewed changes

shajrawi force-pushed the vtable_entry_cache branch from ccf987e to 684f04a Compare September 29, 2016 18:05

add a sidetable cache in the module for VTable entries, replacing lin…

9b2c0cd

…ear lookup

shajrawi force-pushed the vtable_entry_cache branch from 684f04a to 9b2c0cd Compare September 29, 2016 18:07

shajrawi merged commit 00d10c4 into swiftlang:master Sep 29, 2016

shajrawi deleted the vtable_entry_cache branch September 29, 2016 19:17

		llvm::DenseSet<ClassDecl*> vtableClasses;
		size_t EntriesSZ = 0;

Add a sidetable cache in the module for VTable entries, replacing linear lookup #5053

Add a sidetable cache in the module for VTable entries, replacing linear lookup #5053

Uh oh!

Conversation

shajrawi commented Sep 28, 2016

Uh oh!

shajrawi commented Sep 28, 2016

Uh oh!

shajrawi commented Sep 28, 2016

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

eeckstein Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eeckstein Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swiftix commented Sep 28, 2016

Uh oh!

shajrawi commented Sep 28, 2016

Uh oh!

shajrawi commented Sep 28, 2016

Uh oh!

shajrawi commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shajrawi commented Sep 29, 2016

Uh oh!

lattner commented Sep 29, 2016

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eeckstein Sep 28, 2016 •

edited

Loading

eeckstein Sep 28, 2016 •

edited

Loading

gottesmm Sep 29, 2016 •

edited

Loading