It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Fixes b/171917546

Reading the code in the VM, it seems like it is a fatal error if the pthread_setschedparam fails for any reason. The most likely cause of failure is EPERM as the process may only be allowed to set a priority within a certain range. I'd feel a lot better if the failure was not fatal. WDYT @a-siva, @rmacnak-google ?

Having said that, I have only observed EPERMs when attempted to increase the priority of a thread past some value, never when decreasing priorities. So this is probably safe.

/cc @mkustermann as well

haha it failed on Windows: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8863706972037636656/+/steps/Host_Tests_for_host_debug/0/stdout#L22032_0 much like you're afraid of Chinmay.

Error code 87 is invalid parameter.

Hmm. It looks like we can't really set this value in a platform agnostic way, see https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setthreadpriority

Reading the code in the VM, it seems like it is a fatal error if the pthread_setschedparam fails for any reason. The most likely cause of failure is EPERM as the process may only be allowed to set a priority within a certain range. I'd feel a lot better if the failure was not fatal. WDYT @a-siva, @rmacnak-google ?

If it is not a FATAL would we have it return an error? This is in the ThreadStart functionality and not sure if anybody looks at the return value from this funciton.

You can see some discussion on b/154918152 about it. We've decided for this less than ideal solution (whether this functionality is actually used I don't know).

As the example in this bug exemplifies: By default, a pthread cannot lower its priority and later on increase it to its original value. This is add odds with thread pools: We assume we can run arbitrary tasks on our thread pools, if a thread is asked to execute task A and once it's idle execute a task B we may want to lower priority for A but cannot increase priority again for B. So it would prevent us from re-using those threads in a pool. So we have to make it an opt-in feature.

The FATAL was a conscious decision to not silently ignore failures (neither bugs like invalid parameters nor missing permission) of setting the priority: If a customer specifies the flag he wants a certain behavior. If the VM cannot implement that behavior, it should make the customer aware of it. For example: The customer specifies an increased priority which we fail to be able to set, the FATAL will tell about it and the customer might change running of the process with the CAP_SYS_NICE capability.

Now it happens that the user mainly interested in thread priorities happens to have this capability. So for this customer we can arbitrarily change priorities (increase and decrease). That brings up the question how priorities should be set: If we inherit priority from the thread that triggers a task we might get inconsistent behavior (if GC was triggered by allocation failure from UI thread it might have high priority, if GC was triggered from background compiler thread it might have low priority -- though we want the GC to have same priority irrespective of which thread had allocation failure, because GC affects all threads no matter which thread triggered it). So we've decided to go with a simple solution to let embedder tell VM which exact priority to use for VM threads.

Sorry if this is off-topic, but has any thought been given to allowing the Dart VM embedder to supply the threads? Or to supply a callback that accepts information about the VM task (e.g. the priority the VM thinks it should have, maximum tolerable slack, etc.), so that the embedder can get the task scheduled on the embedder-owned thread with the right priority and/or scheduling?

Asking for more clarification from customer if this actually resolves their issue.

Filed dart-lang/sdk#44228 to follow up on requests to Dart API changes.

Closing for now until we have a clearer path forward on this and clarification from customer about whether this actually fixes anything.