[PATCH] D59130: [llvm][Support] Provide interface to set thread priorities

Mon Mar 18 09:32:27 PDT 2019

jfb added a comment.

In D59130#1432873 <https://reviews.llvm.org/D59130#1432873>, @gribozavr wrote:

> In D59130#1426403 <https://reviews.llvm.org/D59130#1426403>, @jfb wrote:
>
> > In D59130#1426086 <https://reviews.llvm.org/D59130#1426086>, @gribozavr wrote:
> >
> > > In D59130#1424778 <https://reviews.llvm.org/D59130#1424778>, @jfb wrote:
> > >
> > > > That doesn't answer my question though: if someone sets priority and on the platform the test witness acceptable forward progress, will it happen that another platform provide no forward progress for the same work. For example, some implementations don't let a background thread perform any I/O. In such a case, a mere logging statement would halt the background thread's progress.
> > >
> > >
> > > Fair enough.  But is this a theoretical concern for some abstract platform that might implement this function in future, or is it a concrete concern for one of the implementations in this review?
> >
> >
> > You're adding a "portable" abstraction to different platform APIs. The onus here is on you to make sure that the abstraction is portable
>
>
> There is a reasonable limit to portability.

Sure, but I'm asking fro something simple here, and I think it's reasonable: please do the research required to show that the supported platforms behave the same. Document what the acceptable and unacceptable uses of the new API are. Are there gotchas? What are their symptoms?

I ask because oftentimes if things like these add a bug it'll manifest weeks later, on someone else's machine, because the new API was used in a new and exciting way which the author didn't foresee. You (the person LGTM'ing the code, and the person who wrote it) won't be the ones debugging the new issue. You're passing on a cost to someone else.

So yeah, there's a reasonable limit to portability, but don't foist a cost on others. Do a bit of homework is all I ask.

> For example, someone might come along and object to filesystem APIs that traverse directories on the basis that there can be a system that does not support nested directories, or uses a content-addressable filesystem.  (Both of those exist in a real world BTW).  However, that is not a valid basis for not adding such APIs.

Agreed that your example isn't relevant to this patch.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59130/new/

https://reviews.llvm.org/D59130