Page MenuHomePhabricator

Use lazy-loading of Metadata in MetadataLoader when importing is enabled (NFC)

Authored by mehdi_amini on Dec 25 2016, 10:41 PM.



This is a relatively simple scheme: we use the index emitted in the
bitcode to avoid loading all the global metadata. Instead we load
the index with their position in the bitcode so that we can load each
of them individually. Materializing the global metadata block in this
condition only triggers loading the named metadata, and the ones
referenced from there (transitively). When materializing a function,
metadatas from the global block are loaded lazily as they are

Two main current limitations are:

  1. Global values other than functions are not materialized on demand,

so we need to eagerly load METADATA_GLOBAL_DECL_ATTACHMENT records
(and their transitive dependencies).

  1. When we load a single metadata, we don't recurse on the operands,

instead we use a placeholder or a temporary metadata. Unfortunately
tepmorary nodes are very expensive. This is why we don't have it
always enabled and only for importing.

These two limitations can be lifted in a subsequent improvement if

With this change, the total link time of opt with ThinLTO and Debug
Info enabled is going down from 282s to 224s (~20%).

Diff Detail


Event Timeline

mehdi_amini retitled this revision from to Use lazy-loading of Metadata in MetadataLoader when importing is enabled (NFC).
mehdi_amini updated this object.
mehdi_amini added reviewers: pcc, tejohnson, dexonsmith.
mehdi_amini added a subscriber: llvm-commits.

Improve comments here and there + very minor improvements

More cleanup and comments!

Fix one last comment!

This looks in good shape now!

469 ↗(On Diff #82490)

This is a refactoring, since there are two places it needs to be called now.

tejohnson edited edge metadata.Dec 29 2016, 10:21 AM

I need to read it through again, but a few comments/questions based on my first pass.

Nit: since "data" is plural I also tend to think of "metadata" as being plural (as well as singular as "data" is commonly used as a singular today). So personally I prefer metadata over metadatas. I noticed a lot of places in the patch where "metadatas" is used.

95 ↗(On Diff #82645)

Can there be a test added based on these strings and the option added below to verify e.g. that the number of records loaded is lower in the lazy-loading case?

414 ↗(On Diff #82645)

s/associated to/associated with/

624 ↗(On Diff #82645)


625 ↗(On Diff #82645)

Should NumMDRecordLoaded be incremented in this case too? Maybe it would be better to CSE the increment of that down into a single place at the end of the BitstreamEntry::Record block (i.e. after the switch on Code, so after any case that doesn't cause an early return)?

671 ↗(On Diff #82645)

s/give on/give up on/

1501 ↗(On Diff #82645)

This is largely the same as the code in parseMetadataStrings. Could it be merged with a callback to do the right thing with each resulting StringRef?

Also, since we are essentially always parsing all the strings, what is the cost of always adding them to the MetadataList as MDStrings? Is this a memory savings?

700 ↗(On Diff #82645)

s/materialize/materializing/ and maybe "all the required globals"?

701 ↗(On Diff #82645)

Maybe "all the required metadata" (since not all metadata are loaded now)?

mehdi_amini marked 6 inline comments as done.Dec 29 2016, 10:34 AM
mehdi_amini added inline comments.
625 ↗(On Diff #82645)

Good point!

1501 ↗(On Diff #82645)

We'd need to hit the context to add the string to a map and unique them there at the same time. I figured that if we load a single function for importing we likely want a tiny portion of the MDString.

mehdi_amini marked 2 inline comments as done.
mehdi_amini edited edge metadata.

Address comments:

  • typos
  • refactor MDString indexing using a callback to differentiate between parsing and indexing.
  • Add a test showing the lazy-loading in action using the statistics.
mehdi_amini marked 3 inline comments as done.Dec 29 2016, 12:29 PM

Test cleanup

Post-break Ping :)

tejohnson added inline comments.Jan 4 2017, 9:59 AM
468 ↗(On Diff #82690)

Use "///" instead for doxygen

615 ↗(On Diff #82690)

Do we know if it is distinct or not?

771 ↗(On Diff #82690)

I had to parse this condition a few times - might be worth a comment. Basically it is looking to see if we have left a temporary, in which case we need to parse the metadata record, otherwise we have a valid node of some kind, right?

782 ↗(On Diff #82690)

Maybe add "/* ModuleLevel */" in front of "false" param. But why is this hardcoded to false? Aren't we module level if we are lazy loading?

787 ↗(On Diff #82690)

From the name and the param, it sounds like this routine is only flushing the placeholders. Suggest renaming to something like resolveForwardRefsAndPlaceholders?

mehdi_amini updated this revision to Diff 83115.Jan 4 2017, 1:17 PM
mehdi_amini marked 5 inline comments as done.

Address comments

615 ↗(On Diff #82690)

The "IsDistinct" used to decide for placeholder is referring to the parent of the metadata we're about to load. NamedMD are always "distinct" AFAIK.
The issue however is that placeholder works only with MDNode which NamedMD are not. So while conceptually possible it requires some deeper change (I'll update the comment, I wrote it before figuring out the limitation)

771 ↗(On Diff #82690)

That was the intent but I don't think it is useful, it is supposed to be already checked at call sites. So I replaced it with an assertion.

782 ↗(On Diff #82690)

I removed the "ModuleLevel" parameter from parseOneMetadata, it was more useful originally but no longer.

tejohnson accepted this revision.Jan 4 2017, 2:29 PM
tejohnson edited edge metadata.


This revision is now accepted and ready to land.Jan 4 2017, 2:29 PM
This revision was automatically updated to reflect the committed changes.