[PATCH] Teach Clang how to use response files when calling other tools

[Rafael Auler]

Hi Sean,

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.

Thanks for putting in time to see this,
Rafael


On Tue, Aug 26, 2014 at 7:11 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
>
> On Mon, Aug 25, 2014 at 7:56 PM, Rafael Auler <rafaelauler at gmail.com>
> wrote:
>
>> Hi Sean,
>>
>> 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:
>>
>> 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.
>>
>> Another important point is that, currently, the Command class stores
>> arguments as a list of char *.
>>
>> 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.
>>
>> 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.
>>
>
> 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.
>
> 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.
>
> 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.
> If you just don't have time or interest in doing the refactoring, that's
> also fine, but please say so.
>
> -- Sean Silva
>
>
>
>>
>> Regards,
>> Rafael
>>
>> ================
>> Comment at: lib/Driver/Job.cpp:122
>> @@ +121,3 @@
>> +/// will escape the regular backslashes (used in Windows paths) and
>> quotes.
>> +static std::unique_ptr<char[]> FlattenArgs(const char **args) {
>> +  // First, determine the length of the command line.
>> ----------------
>> silvas wrote:
>> > This would be a lot nicer with ArrayRef<const char *>; that way, you
>> won't lose the length.
>> 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.
>
>
>> ================
>> Comment at: lib/Driver/Job.cpp:169
>> @@ +168,3 @@
>> +  llvm::SmallVector<const char *, 128> NewArgv;
>> +  unsigned len = 0;
>> +  for (auto Input : Inputs) {
>> ----------------
>> silvas wrote:
>> > Check your naming here and in other places.
>> Thanks for pointing out, I fixed the naming. Will submit the patch soon.
>>
>> ================
>> Comment at: lib/Driver/Tools.h:546-550
>> @@ +545,7 @@
>> +    llvm::sys::EncodingMethod getResponseFileEncoding() const override {
>> +#if defined(LLVM_ON_WIN32)
>> +      return llvm::sys::EM_CurrentCodePage;
>> +#else
>> +      return llvm::sys::EM_UTF8;
>> +#endif
>> +    }
>> ----------------
>> silvas wrote:
>> > Ew... let's not have a bunch of random #ifdef's all over the code
>> outside libSupport.
>> 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>.
>>
>> 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.
>>
>> http://reviews.llvm.org/D4897
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140826/337642b3/attachment.html>