<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 4, 2017 at 2:35 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>> It might do, keeping in mind that reading pretty much every existing object<br>
> file format already requires scanning for string lengths. Certainly<br>
> something to try and evaluate, at least.<br>
<br>
</span>In lld strlen does show up in the profile. I haven't benchmarked it<br>
recently enough to remember exactly how much.<br></blockquote><div><br></div><div>So I have spent some time evaluating the various options that have been discussed on this thread. They are:</div><div>1) adding the string table itself (this involves moving all data in the VST to either the string table or elsewhere in the bitcode, with the exception of function offsets)</div><div>2) representing string references as offset/size</div><div>3) removing VST function offsets (and the VST itself)</div><div><br></div><div>I am attaching three patches with my prototypes, where t1 implements the string table, t2 is string table + remove function offsets and t3 is string table + remove function offsets + offset/size. (They are all against r299588.) I used these patches to evaluate the perf impact when linking Chromium with ThinLTO.</div><div><br></div><div>Median time elapsed for a no-op incremental link (#runs = 100):</div><div>- trunk: 51.71s</div><div>- string table: 48.66s (-5.9%)</div><div>- string table + remove function offsets: 49.12s (-5.0%)</div><div>- string table + remove function offsets + offset/size: 48.10s (-7.0%)</div><div><br></div><div>And for a full link (#runs = 13):</div><div>- trunk: 605.24s</div><div>- string table: 597.88s (-1.3%)</div><div>- string table + remove function offsets: 598.99s (-1.1%)</div><div>- string table + remove function offsets + offset/size: 598.97s (-1.1%)</div><div><br></div><div>It is hard to draw any conclusions from the full ThinLTO links (partly due to the small #runs) other than that adding the string table is a clear win. But I think it is clear enough from the incremental results that offset/size is a win. And as Duncan mentioned it should allow us to more easily share strings with the metadata string table.</div><div><br></div><div>Function offsets do appear to be a win, so it seems likely that we will want something like them, either in the symbol table or elsewhere. My conclusion is that there should be no great harm in storing just the function offsets in the VST for now because, after we add the string table, the data in the VST will no longer be needed for correctness and any potential future change that moves the function offsets elsewhere can simply start ignoring the VST in the reader.</div><div><br></div><div>I also measured the impact on file size by taking the total sum of object file sizes for each of the variants. They are:</div><div>- trunk: 1183510504 bytes<br></div><div>- string table: 1151533664 bytes (-2.7%)</div><div>- string table + remove function offsets: 1147981736 bytes (-3.0%)</div><div>- string table + remove function offsets + offset/size: 1149361632 bytes (-2.9%)</div><div>The decrease in size vs trunk is expected because of sharing of strings between comdats and globals as well as between modules. But the differences in sizes between the different options does not seem as important.</div><div><br></div><div>So here is what I plan to do:</div><div>- Create a new patch that implements string table + offset/size and take further measurements, to make sure that removing function offsets is not a confounding factor for performance.</div><div>- Clean up the patch and send it for review.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The string table builder code currently supports tail merging. On ELF<br>
at least that is a very modest size saving. If the size is written as<br>
a prefix to the string, that doesn't work. If the size is stored in<br>
the reference (like a stringef) we would be able to merge any<br>
substring (not sure if it is profitable).<br></blockquote><div><br></div><div>I agree that we should store the size in the reference. Tail merging should help on targets that require IR name mangling because the mangled name should almost always be a suffix of the unmangled name or vice versa.</div><div><br></div><div>Thanks,</div></div>-- <br><div class="gmail-m_6970628451836088677m_-5410130619254783602gmail-m_332212620201084017m_-9001923105225869334m_9166763130011845098m_-5048238655524626374m_1019773840393323610gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div></div>