aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Interpreter/Value.h:160-162
+ // Interpreter, QualType are stored as void* to reduce dependencies.
+ void *Interp = nullptr;
+ void *OpaqueType = nullptr;
----------------
junaire wrote:
> v.g.vassilev wrote:
> > sgraenitz wrote:
> > > v.g.vassilev wrote:
> > > > aaron.ballman wrote:
> > > > > junaire wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > v.g.vassilev wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > v.g.vassilev wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > Why don't forward declares suffice if we're storing the
> > > > > > > > > > > information by pointer?
> > > > > > > > > > This is a performance-critical class. We literally measure
> > > > > > > > > > the instruction count for it. We practically cannot include
> > > > > > > > > > anything in this header file because the class needs to
> > > > > > > > > > included as part of the interpreter runtime. For example:
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > #include <clang/Interpreter/Value.h>
> > > > > > > > > > Value ResultV;
> > > > > > > > > > gInterpreter->evaluate("float i = 12.3; i++", &V);
> > > > > > > > > > printf("Value is %d\n", ResultV.castAs<int>());
> > > > > > > > > > ```
> > > > > > > > > >
> > > > > > > > > > This is how you can do things in Cling. This not yet there
> > > > > > > > > > but that's our next step.
> > > > > > > > > >
> > > > > > > > > > For performance reasons we have the `ValueKind`
> > > > > > > > > > optimization which allows us to perform most of the
> > > > > > > > > > operations we need very fast. There are some operations
> > > > > > > > > > such as printing the concrete type which need the actual
> > > > > > > > > > `QualType` and so on but they are outside of the
> > > > > > > > > > performance critical paths and it is okay to resort back to
> > > > > > > > > > the real types providing the level of accuracy we need.
> > > > > > > > > >
> > > > > > > > > That sounds like it's going to lead to maintenance problems
> > > > > > > > > in the long term, right? I can't think of another header file
> > > > > > > > > we say "don't touch this because it may impact runtime
> > > > > > > > > performance", and so I can easily imagine someone breaking
> > > > > > > > > your expectation that this file can't include anything else.
> > > > > > > > >
> > > > > > > > > Is there a long-term plan for addressing this?
> > > > > > > > We have a few components like the Lexer that are extremely
> > > > > > > > prone to performance regressions.
> > > > > > > >
> > > > > > > > In terms for a longer-term plan in addressing this there are
> > > > > > > > some steps could help IMO. First, this component is relatively
> > > > > > > > standalone and very few changes will be required over time, for
> > > > > > > > these I am hoping to be listed as a reviewer. Second, we can
> > > > > > > > add a comment in the include area, making a note that including
> > > > > > > > anything here will degrade the performance of almost all
> > > > > > > > interpreted code. Third, we will find out about this in our
> > > > > > > > downstream use-cases as the things get significantly slower.
> > > > > > > >
> > > > > > > > Neither is silver bullet but that's probably the best we could
> > > > > > > > do at that time. Btw, we might be able to add a test that's
> > > > > > > > part of LLVM's performance analysis infrastructure.
> > > > > > > > Neither is silver bullet but that's probably the best we could
> > > > > > > > do at that time. Btw, we might be able to add a test that's
> > > > > > > > part of LLVM's performance analysis infrastructure.
> > > > > > >
> > > > > > > Yeah, we should probably consider doing that. But to make sure I
> > > > > > > understand the performance concerns... when we change
> > > > > > > functionality in the lexer, we (potentially) slow down the lexing
> > > > > > > phase of compilation. That's straightforward and unsurprising.
> > > > > > > But in this case, it sounds like the act of including another
> > > > > > > header file in this header file will cause a runtime performance
> > > > > > > concern, even if no other changes are made. If I'm correct, I
> > > > > > > can't think of anything else in the compiler that works like that.
> > > > > > I believe what @v.g.vassilev means is that the repl itself might
> > > > > > include `Value.h` as a part of *runtime*, so if the header is
> > > > > > heavy, you can notice the repl is slowed down. (That's obvious) So
> > > > > > keep in mind we're breaking the boundary between the compiled code
> > > > > > and interpreted code (It's kinda confusing) here it actually
> > > > > > impacts interpreted code.
> > > > > > I believe what @v.g.vassilev means is that the repl itself might
> > > > > > include Value.h as a part of *runtime*, so if the header is heavy,
> > > > > > you can notice the repl is slowed down. (That's obvious) So keep in
> > > > > > mind we're breaking the boundary between the compiled code and
> > > > > > interpreted code (It's kinda confusing) here it actually impacts
> > > > > > interpreted code.
> > > > >
> > > > > I'm not certain that's a reasonable design choice to make. Or, stated
> > > > > somewhat differently, I'm really uncomfortable with having header
> > > > > files we can't maintain because changing them impacts runtime
> > > > > performance in surprising ways. That's not a sustainable design even
> > > > > if we think this header will be reasonably stable. We need *some*
> > > > > amount of abstraction here so that we can have a clean design for the
> > > > > REPL interpreter without NFC code changes impacting performance.
> > > > > Otherwise, people will be discouraged from adding comments to this
> > > > > file (those take time to lex, after all), or using long identifiers
> > > > > (longer identifiers take longer to lex than shorter ones), or
> > > > > including what is used instead of using `void *` (as being discussed
> > > > > here), and so on.
> > > > >
> > > > > This is quite probably something you've already thought about plenty,
> > > > > but... could we add an abstraction layer so that the interpreter side
> > > > > of things has a "low-token-count" interface that dispatches through
> > > > > to the actual implementation?
> > > > > > I believe what @v.g.vassilev means is that the repl itself might
> > > > > > include Value.h as a part of *runtime*, so if the header is heavy,
> > > > > > you can notice the repl is slowed down. (That's obvious) So keep in
> > > > > > mind we're breaking the boundary between the compiled code and
> > > > > > interpreted code (It's kinda confusing) here it actually impacts
> > > > > > interpreted code.
> > > > >
> > > > > I'm not certain that's a reasonable design choice to make. Or, stated
> > > > > somewhat differently, I'm really uncomfortable with having header
> > > > > files we can't maintain because changing them impacts runtime
> > > > > performance in surprising ways. That's not a sustainable design even
> > > > > if we think this header will be reasonably stable. We need *some*
> > > > > amount of abstraction here so that we can have a clean design for the
> > > > > REPL interpreter without NFC code changes impacting performance.
> > > > > Otherwise, people will be discouraged from adding comments to this
> > > > > file (those take time to lex, after all), or using long identifiers
> > > > > (longer identifiers take longer to lex than shorter ones), or
> > > > > including what is used instead of using `void *` (as being discussed
> > > > > here), and so on.
> > > >
> > > > All valid points. I guess we have seen some changes related to
> > > > compilation speed in the past in the STLExtras.h (iirc, although I
> > > > cannot find the right commit). We did particular changes to the header
> > > > file to reduce the compilation time of some large TU builds. I'd think
> > > > that's more like the case of `stddef.h` and similar headers in the
> > > > resource directory. The more we add the worse becomes the compiler
> > > > startup time.
> > > >
> > > > >
> > > > > This is quite probably something you've already thought about plenty,
> > > > > but... could we add an abstraction layer so that the interpreter side
> > > > > of things has a "low-token-count" interface that dispatches through
> > > > > to the actual implementation?
> > > >
> > > > Yes, I have a plan that's quite ambitious (and a draft RFC): basically
> > > > the idea is any `#include` to become a no-op for the compiler unless
> > > > something is actually needed.
> > > >
> > > > I understand your concern here but I don't really know how to address
> > > > it in this particular patch.
> > > >
> > > >
> > > >> [...] include Value.h as a part of *runtime*, so if the header is
> > > >> heavy, you can notice the repl is slowed down. (That's obvious) So
> > > >> keep in mind we're breaking the boundary between the compiled code and
> > > >> interpreted code (It's kinda confusing) here it actually impacts
> > > >> interpreted code.
> > > > I'm really uncomfortable with having header files we can't maintain
> > > > because changing them impacts runtime performance in surprising ways.
> > > > [...] could we add an abstraction layer so that the interpreter side of
> > > > things has a "low-token-count" interface that dispatches through to the
> > > > actual implementation?
> > >
> > > Agree. What about splitting this up in 3 parts?
> > > (1) Private API: interface as consumed by the libclangInterpreter
> > > (2) Public API: interface as consumed by tools, other LLVM projects and
> > > out-of-tree
> > > (3) Client API: the actual runtime-only parts with low-token-count etc.
> > >
> > > (1) and (2) would be here in the source-tree. (3) is a resource file
> > > similar to intrinsics headers, I believe they have very similar
> > > requirements on performance, maintenance and versioning.
> > I agree with this suggestion. We have actually tried `(3)` and discovered
> > that it is challenging to write unit tests for it. The main issue is that
> > the clang tests have been designed to avoid relying on the resource
> > directory, which is relative to the clang binary. Moreover, some builds
> > redefine the location of the resource directory during compilation, making
> > it difficult to locate and pass to a unit test whose location is
> > independent of the clang binary's location...
> Hi @aaron.ballman, I would like to try to explain why we don't have a better
> approach again.
>
> `Value` is very special, it's not a regular runtime that would only be used
> by the REPL, instead, it acts like a messager, and connects the compiled code
> and the interpreted code. That said both sides need to reference it. so it
> can't be put into the resource directory, or the host compiler that is used
> for compiling clang-repl can't find it, then we fail to compile. What's more,
> it can't be put into compiler-rt too, same reason, `Interpreter.h` needs to
> include the header. The only possible location I can think of is the regular
> `include/clang/Interpter`.
>
> When it comes to maintenance issues, I believe that's fine. IIUC your main
> concern is that people will be afraid to touch this file since it has a
> surprising performance impact. However, the whole header is only about the
> declaration of `Value` class and it should only be used for this purpose. So
> unless someone is working on clang-repl, it's unlikely for them to touch it.
> Another thing I would like to mention is that the performance is only
> noticeably dropped if we pulled some large headers, like `<string>`,
> `<memory>`, or something like that. NFC changes like fixing typos, adding
> comments, and adjusting function names are totally fine. (They can't simply
> refactor the `void*` thing, or the whole program simplify fail to compile)
>
> We could also add some comments to indicate the specificity of this header,
> and why people should be cautious when touching it.
>
>
I had a nice off-list conversation with @v.g.vassilev about my concerns and we
think we've got a path forward to get you unblocked.
Put a gigantic, obvious note at the top of Value.h saying that 1) changes to
this file impact *runtime* performance of the interpreter and so extreme
caution should be used when making changes to the file, especially when
including new headers., 2) A FIXME comment that explains why this design is a
problem and what the very vague idea is for addressing the issue, and that the
contents of this file are experimental and subject to change.
#1 helps code reviewers remember that this file is special right now, and #2
makes it clear that nobody should rely on the API being stable because we're
hopefully going to make significant changes to it in the relatively near future.
(Note: one of the conversation topics was that there is interest in running the
interpreter on embedded systems, which is another compelling reason that we'll
need a less token-heavy interface for the performance sensitive parts.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141215/new/
https://reviews.llvm.org/D141215
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
