Supporting multiple instances of --js-flags

[Omer Katz]

Hi folks,

Chrome generally assumes that command line flags are only passed once.

Whenever a flag is passed multiple times, only the last instance is kept and all previous instances are ignored.

This has proven problematic for V8 on several occasions.

Some benchmarks implicitly use `--js-flags` to set V8 flags.

V8 folks also often use `--js-flags` to enable/disable V8 features for debugging and performance evaluations.

When used at the same time, one instance is dropped, leading to either crashes or false/misleading results.

I looked into revising the handling such that multiple instances of `--js-flags` would be concatenated rather than ignored, and write a short proposal doc:

https://docs.google.com/document/d/1FR4e2AO0eWKPz84XfO9gKDZTi0w0S_jerCJMD9u6wjw

I'd appreciate any feedback and opinions on it.

Thanks,

Omer

--

Omer Katz

V8, Software Engineer

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. 

     

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Kentaro Hara]

If this behavior change is limited to --js-flags (instead of all flags in general), and you are sure that the change is not likely to cause compatibility issues on existing tools, LGTM :)

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAJFGqfObcPx_VJ7rtY0ECr_8TdGCmOJOkRzvPz5eTq2Zb%3DF%3DjQ%40mail.gmail.com.

--

Kentaro Hara, Tokyo

[danakj]

Using a global variable to do this seems like a fairly heavy hammer with difficult action-at-a-distance implications, can we instead add a CommandLine method that merges them when querying for the flag, and then use that for --js-flags?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jzgFmBe_NOj%2B7w7YBp_nDxOZgwhRej4AHXpqCZONydLEg%40mail.gmail.com.

[Omer Katz]

I agree that it's a rather large hammer and it wasn't my first choice, but I think it's what can work with our current design.

If we could merge on querying that would be great and preferable. Unfortunately the way our CommandLine class works atm is that it parses argv on initialization and discards of duplicates at that time. It then caches a mapping from flag to value and all other discarded values are no longer available.

Changing the CommandLine design is a larger endeavor that I don't think we embark on.

Does that make sense?

[danakj]

On Thu, Jul 11, 2024 at 1:34 PM Omer Katz <omer...@google.com> wrote:

I agree that it's a rather large hammer and it wasn't my first choice, but I think it's what can work with our current design.

If we could merge on querying that would be great and preferable. Unfortunately the way our CommandLine class works atm is that it parses argv on initialization and discards of duplicates at that time. It then caches a mapping from flag to value and all other discarded values are no longer available.

Changing the CommandLine design is a larger endeavor that I don't think we embark on.

Does that make sense?

Yeah that makes sense. Can we CHECK that the DuplicateSwitchHandler is set only one time, and never set after CommandLine is initialized? I want to avoid the situation where tests are overriding this and then breaking the production behaviour, and there's no way to hear about it, or vice versa. Or that it's set late and then doesn't work as intended, etc.

I also think the whole thing is a bit over-engineered with virtual methods on heap-allocated class objects instead of just setting a list of flags, but I think that can be a separate concern.

[Omer Katz]

On Mon, Jul 15, 2024 at 3:57 PM danakj <dan...@chromium.org> wrote:

On Thu, Jul 11, 2024 at 1:34 PM Omer Katz <omer...@google.com> wrote:

I agree that it's a rather large hammer and it wasn't my first choice, but I think it's what can work with our current design.

If we could merge on querying that would be great and preferable. Unfortunately the way our CommandLine class works atm is that it parses argv on initialization and discards of duplicates at that time. It then caches a mapping from flag to value and all other discarded values are no longer available.

Changing the CommandLine design is a larger endeavor that I don't think we embark on.

Does that make sense?

Yeah that makes sense. Can we CHECK that the DuplicateSwitchHandler is set only one time, and never set after CommandLine is initialized? I want to avoid the situation where tests are overriding this and then breaking the production behaviour, and there's no way to hear about it, or vice versa. Or that it's set late and then doesn't work as intended, etc.

I think that is a reasonable guarantee to have, and in production it should definitely work. We can even go a step further and mark the existing `CommandLine::SetDuplicateSwitchHandler` method as `ForTesting` so that it's not used outside of tests.

For tests it seems a bit more complicated. Our current testing infra supports running multiple tests in the same process in parallel on multiple threads. The duplicate switch handler is currently process-global. Looking at tests in command_line_unittest.cc, there's one test (MultipleFilterFileSwitch) there that sets a handler. That test could run while a CommandLine is live in another test on another thread. I think that kind of situation would inherently violate a check for "the DuplicateSwitchHandler is never set after CommandLine is initialized" (and we'd observe flaky crashes because of it).

What we more easily can do for tests is check that we never set a non-default handler while there's already one set, and never set a handler when we have a command line object initialized for the process (should cover all currently existing cases other than MultipleFilterFileSwitch).

Wdyt?

Btw, it's cleaner to implement the checks if instead of introducing a default DuplicateSwitchHandler we just hard code the default behavior, so I've implemented that in patchset 6 and updated the doc accordingly.

[Omer Katz]

On Tue, Jul 16, 2024 at 1:36 PM Omer Katz <omer...@google.com> wrote:

On Mon, Jul 15, 2024 at 3:57 PM danakj <dan...@chromium.org> wrote:

On Thu, Jul 11, 2024 at 1:34 PM Omer Katz <omer...@google.com> wrote:

I agree that it's a rather large hammer and it wasn't my first choice, but I think it's what can work with our current design.

If we could merge on querying that would be great and preferable. Unfortunately the way our CommandLine class works atm is that it parses argv on initialization and discards of duplicates at that time. It then caches a mapping from flag to value and all other discarded values are no longer available.

Changing the CommandLine design is a larger endeavor that I don't think we embark on.

Does that make sense?

Yeah that makes sense. Can we CHECK that the DuplicateSwitchHandler is set only one time, and never set after CommandLine is initialized? I want to avoid the situation where tests are overriding this and then breaking the production behaviour, and there's no way to hear about it, or vice versa. Or that it's set late and then doesn't work as intended, etc.

I think that is a reasonable guarantee to have, and in production it should definitely work. We can even go a step further and mark the existing `CommandLine::SetDuplicateSwitchHandler` method as `ForTesting` so that it's not used outside of tests.

For tests it seems a bit more complicated. Our current testing infra supports running multiple tests in the same process in parallel on multiple threads. The duplicate switch handler is currently process-global. Looking at tests in command_line_unittest.cc, there's one test (MultipleFilterFileSwitch) there that sets a handler. That test could run while a CommandLine is live in another test on another thread. I think that kind of situation would inherently violate a check for "the DuplicateSwitchHandler is never set after CommandLine is initialized" (and we'd observe flaky crashes because of it).

What we more easily can do for tests is check that we never set a non-default handler while there's already one set, and never set a handler when we have a command line object initialized for the process (should cover all currently existing cases other than MultipleFilterFileSwitch).

Wdyt?

Fyi, it turns out we already have tests that set the DuplicateSwitchHandler too late such that it has no impact. Luckily we can easily avoid it by not setting the handler when the command line has already been initialized (patchset 8).

[Omer Katz]

Please ignore my last 2 messages. I was too quick to assume everything works.

While I think the expectations that we don't set a handler after initializing the command line, or not set a non-default handler while there is already one set, are reasonable expectations, I'm afraid that's not how our current infrastructure works.

On ToT we currently have many tests that set a handler after the command line was initialized (and are apparently unaffected by it, but the handler is set by a shared utility class), and the one test (MultipleFilterFileSwitch) that also overrides another non-default handler.

I was looking through the code and, assuming we want to keep the tests, I don't think we can avoid these issues without redesigning how we deal with the command line (which I don't think we should do).

None of this happens in production code, but it appears to be unavoidable at the moment for our testing code.

If you have any ideas I may have missed please let me know, otherwise I think we will be forced to go on without the guarantees.

[danakj]

On Tue, Jul 16, 2024 at 10:30 AM Omer Katz <omer...@google.com> wrote:

Please ignore my last 2 messages. I was too quick to assume everything works.

While I think the expectations that we don't set a handler after initializing the command line, or not set a non-default handler while there is already one set, are reasonable expectations, I'm afraid that's not how our current infrastructure works.

On ToT we currently have many tests that set a handler after the command line was initialized (and are apparently unaffected by it, but the handler is set by a shared utility class), and the one test (MultipleFilterFileSwitch) that also overrides another non-default handler.

This sounds really buggy, it's great to have found it. Shall we fix it, since we're making other changes in the same area now?

 

I was looking through the code and, assuming we want to keep the tests, I don't think we can avoid these issues without redesigning how we deal with the command line (which I don't think we should do).

It sounds like we need to organize the setting of configs before the CommandLine, rather than change how CommandLine itself works.

But another option would be to get rid of all these virtual objects and have CommandLine hold a multimap. The default behaviour can still return the last flag that was seen, so the APIs don't change. But we could provide a different method to grab them all.

[Omer Katz]

On Tue, Jul 16, 2024 at 4:57 PM danakj <dan...@chromium.org> wrote:

On Tue, Jul 16, 2024 at 10:30 AM Omer Katz <omer...@google.com> wrote:

Please ignore my last 2 messages. I was too quick to assume everything works.

While I think the expectations that we don't set a handler after initializing the command line, or not set a non-default handler while there is already one set, are reasonable expectations, I'm afraid that's not how our current infrastructure works.

On ToT we currently have many tests that set a handler after the command line was initialized (and are apparently unaffected by it, but the handler is set by a shared utility class), and the one test (MultipleFilterFileSwitch) that also overrides another non-default handler.

This sounds really buggy, it's great to have found it. Shall we fix it, since we're making other changes in the same area now?

These issues only exist in tests, so I'm not sure I would refer to it as buggy. The tests are currently working correctly, they're just doing extra redundant work by setting a handler that's never used (because it's set by a shared utility; the tests that actually need the handler do set it in time afaict).

 

I was looking through the code and, assuming we want to keep the tests, I don't think we can avoid these issues without redesigning how we deal with the command line (which I don't think we should do).

It sounds like we need to organize the setting of configs before the CommandLine, rather than change how CommandLine itself works.

I'm not sure what you refer to as "configs" here. I consider the handler config and the CommandLine class as one and the same in this regard. 

But another option would be to get rid of all these virtual objects and have CommandLine hold a multimap. The default behaviour can still return the last flag that was seen, so the APIs don't change. But we could provide a different method to grab them all.

This is what I referred to as redesigning how we deal with the command line.

I think it would be great to do that, but I don't currently have any cycles to work on it.

As much as the silent dropping of duplicates is hurting me/us, if a redesign is the direction we want to go in then I will have to leave it as is for now.

Hopefully I can tackle it again at a later time or someone else would be interested in taking over. 

[danakj]

On Tue, Jul 16, 2024 at 11:06 AM Omer Katz <omer...@google.com> wrote:

On Tue, Jul 16, 2024 at 4:57 PM danakj <dan...@chromium.org> wrote:

On Tue, Jul 16, 2024 at 10:30 AM Omer Katz <omer...@google.com> wrote:

Please ignore my last 2 messages. I was too quick to assume everything works.

While I think the expectations that we don't set a handler after initializing the command line, or not set a non-default handler while there is already one set, are reasonable expectations, I'm afraid that's not how our current infrastructure works.

On ToT we currently have many tests that set a handler after the command line was initialized (and are apparently unaffected by it, but the handler is set by a shared utility class), and the one test (MultipleFilterFileSwitch) that also overrides another non-default handler.

This sounds really buggy, it's great to have found it. Shall we fix it, since we're making other changes in the same area now?

These issues only exist in tests, so I'm not sure I would refer to it as buggy. The tests are currently working correctly, they're just doing extra redundant work by setting a handler that's never used (because it's set by a shared utility; the tests that actually need the handler do set it in time afaict).

This still seems very buggy, it makes it impossible to tell what is going on by reading the code! You've had to do some great debugging to get us this far. And we even see comments in the code like "We want to stop throwing away duplicate test filter file flags, but we're afraid of changing too much in fear of breaking other use cases." which is a big red flag that things are not good right now.

 

I was looking through the code and, assuming we want to keep the tests, I don't think we can avoid these issues without redesigning how we deal with the command line (which I don't think we should do).

It sounds like we need to organize the setting of configs before the CommandLine, rather than change how CommandLine itself works.

I'm not sure what you refer to as "configs" here. I consider the handler config and the CommandLine class as one and the same in this regard. 

I mean setting up the configuration of how the CommandLine will run before it's initialized.

 

But another option would be to get rid of all these virtual objects and have CommandLine hold a multimap. The default behaviour can still return the last flag that was seen, so the APIs don't change. But we could provide a different method to grab them all.

This is what I referred to as redesigning how we deal with the command line.

I think it would be great to do that, but I don't currently have any cycles to work on it.

As much as the silent dropping of duplicates is hurting me/us, if a redesign is the direction we want to go in then I will have to leave it as is for now.

Hopefully I can tackle it again at a later time or someone else would be interested in taking over. 

Hm, it sounds like what we have now was poorly understood and is not working the way the code reads it should work - as we have cases setting a handler that is ignored. It would make me sad to just leave it that way instead of improving it. If you don't have time to work on improving things directly in the area that you're working around, I understand, and I wish things were a bit different.

[Omer Katz]

On Tue, Jul 16, 2024 at 5:12 PM danakj <dan...@chromium.org> wrote:

On Tue, Jul 16, 2024 at 11:06 AM Omer Katz <omer...@google.com> wrote:

On Tue, Jul 16, 2024 at 4:57 PM danakj <dan...@chromium.org> wrote:

On Tue, Jul 16, 2024 at 10:30 AM Omer Katz <omer...@google.com> wrote:

Please ignore my last 2 messages. I was too quick to assume everything works.

While I think the expectations that we don't set a handler after initializing the command line, or not set a non-default handler while there is already one set, are reasonable expectations, I'm afraid that's not how our current infrastructure works.

On ToT we currently have many tests that set a handler after the command line was initialized (and are apparently unaffected by it, but the handler is set by a shared utility class), and the one test (MultipleFilterFileSwitch) that also overrides another non-default handler.

This sounds really buggy, it's great to have found it. Shall we fix it, since we're making other changes in the same area now?

These issues only exist in tests, so I'm not sure I would refer to it as buggy. The tests are currently working correctly, they're just doing extra redundant work by setting a handler that's never used (because it's set by a shared utility; the tests that actually need the handler do set it in time afaict).

This still seems very buggy, it makes it impossible to tell what is going on by reading the code! You've had to do some great debugging to get us this far. And we even see comments in the code like "We want to stop throwing away duplicate test filter file flags, but we're afraid of changing too much in fear of breaking other use cases." which is a big red flag that things are not good right now.

 

I was looking through the code and, assuming we want to keep the tests, I don't think we can avoid these issues without redesigning how we deal with the command line (which I don't think we should do).

It sounds like we need to organize the setting of configs before the CommandLine, rather than change how CommandLine itself works.

I'm not sure what you refer to as "configs" here. I consider the handler config and the CommandLine class as one and the same in this regard. 

I mean setting up the configuration of how the CommandLine will run before it's initialized.

 

But another option would be to get rid of all these virtual objects and have CommandLine hold a multimap. The default behaviour can still return the last flag that was seen, so the APIs don't change. But we could provide a different method to grab them all.

This is what I referred to as redesigning how we deal with the command line.

I think it would be great to do that, but I don't currently have any cycles to work on it.

As much as the silent dropping of duplicates is hurting me/us, if a redesign is the direction we want to go in then I will have to leave it as is for now.

Hopefully I can tackle it again at a later time or someone else would be interested in taking over. 

Hm, it sounds like what we have now was poorly understood and is not working the way the code reads it should work - as we have cases setting a handler that is ignored. It would make me sad to just leave it that way instead of improving it. If you don't have time to work on improving things directly in the area that you're working around, I understand, and I wish things were a bit different.

Don't get me wrong, I totally agree with you here. This was a detour for me so I can't spend too much time on it atm. Let's leave it as is for now and hopefully we can pick it back up soon.