<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 26, 2014 at 3:32 PM, Rafael Auler <span dir="ltr"><<a href="mailto:rafaelauler@gmail.com" target="_blank">rafaelauler@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Sean,<div><br></div><div>I think you misunderstood me, I don't want to avoid refactoring the code, but I was only expressing high level concerns that I had when writing this code that are indeed not backed by real profiling, and would like to know if you share the same concerns or not. Since you do not think that these are problems, I will work on the refactoring and keep you updated.</div>
</div></blockquote><div><br></div><div>Cool. I think it would make sense to do the change to <span style="font-size:13px;font-family:arial,sans-serif">llvm::sys::</span><span style="font-size:13px;font-family:arial,sans-serif">argumentsFitWithinSystemLimits anyways, since currently that code duplicates a lot of the code in this patch (all performance concerns aside).</span></div>
<div><span style="font-size:13px;font-family:arial,sans-serif"><br></span></div><div><span style="font-size:13px;font-family:arial,sans-serif">-- Sean Silva</span></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">
<div><br></div><div>Thanks for putting in time to see this,</div><div>Rafael</div></div><div class=""><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 26, 2014 at 7:11 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Mon, Aug 25, 2014 at 7:56 PM, Rafael Auler <span dir="ltr"><<a href="mailto:rafaelauler@gmail.com" target="_blank">rafaelauler@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Sean,<br>
<br>
Thanks a lot for your careful review. I have answered your questions below, and I have few concerns. Please let me know if I am missing something:<br>
<br>
This code is on the critical path for commands with very large lengths and this is a non-negligible time. For UNIX, for example, this would be triggered for commands spanning more than 2MB of arguments and Clang spends a good amount of time to parse even trivial arguments, because it is a huge number of arguments.<br>
<br>
Another important point is that, currently, the Command class stores arguments as a list of char *.<br>
<br>
This is the reason for this design with lots of naked char *. It avoids creating StringRefs, that would measure the length of char* in their constructor and cause more overhead. It also avoids using high-level classes such as streams and bump allocators because, since this is likely to be called only when the number of arguments is very large, would cause them to repeatedly perform heap allocations until they finally settle to support the final size of the string. Either that or we would need to work with very large slabs, wasting memory. In this design, the length is calculated once and a single allocation is made, similar to what is done in Execute() in Support/Windows/Program.inc.<br>
<br>
Therefore, by calculating the size of the final command line beforehand, even though it looks like we are losing by iterating over all args twice (one for length calculation and other for parsing), we win by eliminating inefficiencies in the allocation mechanism in runtime or in memory consumption.<br>
</blockquote><div><br></div></div><div>Sorry, but this sounds like a self-contradictory mishmash of performance rules of thumb cherry-picked to sound like an excuse for not refactoring the code.</div><div><br></div><div>
It's obvious you haven't profiled this because if you had profiled (and these were actually performance issues), the first thing you would do is modify llvm::sys::argumentsFitWithinSystemLimits to return the required buffer size, since it is already doing this exact length calculation you are doing (or could be easily modified to do it). You would also have considered using a raw_ostream since that bounds the memory consumption by the size of the buffer that it periodically flushes to the OS.</div>
<div><br></div><div>If you seriously want to justify not making that change solely on the basis of performance, please run some concrete benchmarks and profiles and share the data.<br></div><div>If you just don't have time or interest in doing the refactoring, that's also fine, but please say so.<span><font color="#888888"><br>
</font></span></div><span><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><div><div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Regards,<br>
Rafael<br>
<div><br>
================<br>
Comment at: lib/Driver/Job.cpp:122<br>
@@ +121,3 @@<br>
+/// will escape the regular backslashes (used in Windows paths) and quotes.<br>
+static std::unique_ptr<char[]> FlattenArgs(const char **args) {<br>
+ // First, determine the length of the command line.<br>
----------------<br>
</div><div>silvas wrote:<br>
> This would be a lot nicer with ArrayRef<const char *>; that way, you won't lose the length.<br>
</div>That's a good point. However, I am afraid that I would still need to recalculate the length by traversing each argument, calculating its length and assessing whether it needs quoting or not.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><br>
================<br>
Comment at: lib/Driver/Job.cpp:169<br>
@@ +168,3 @@<br>
+ llvm::SmallVector<const char *, 128> NewArgv;<br>
+ unsigned len = 0;<br>
+ for (auto Input : Inputs) {<br>
----------------<br>
</div><div>silvas wrote:<br>
> Check your naming here and in other places.<br>
</div>Thanks for pointing out, I fixed the naming. Will submit the patch soon.<br>
<div><br>
================<br>
Comment at: lib/Driver/Tools.h:546-550<br>
@@ +545,7 @@<br>
+ llvm::sys::EncodingMethod getResponseFileEncoding() const override {<br>
+#if defined(LLVM_ON_WIN32)<br>
+ return llvm::sys::EM_CurrentCodePage;<br>
+#else<br>
+ return llvm::sys::EM_UTF8;<br>
+#endif<br>
+ }<br>
----------------<br>
</div><div>silvas wrote:<br>
> Ew... let's not have a bunch of random #ifdef's all over the code outside libSupport.<br>
</div>I agree that this is inadequate. My previous patch version delegated this reasoning to libSupport. However, it was more difficult to understand and I changed it for clarity. Now, it is obvious that the response file encoding for this tool depends on whether it is running on Windows or not. In my previous libSupport version, I had enum members that meant "this is UTF on UNIX but UTF16 on Windows", and so on. I think this is not a good design because if you need to support a new tool, you may potentially need to add a new enum member in libSupport to encode your tuple <MyUNIXEncoding, MyWindowsEncoding>.<br>
<br>
Since the encoding depends on the tool and on the operating system, I would appreciate if you have any suggestion on how to code this. Meanwhile, I will think in something.<br>
<br>
<a href="http://reviews.llvm.org/D4897" target="_blank">http://reviews.llvm.org/D4897</a><br>
<br>
<br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>