Hi Vasilii,
What do you think about the proposal to make PFM movable and store it in vectors directly?

It is obviously not high priority, but do you consider it a good change, or actually not?

I'd start with making PasswordForm movable. It's a good idea on its own.

const autofill::PasswordForm observed_form_ can be a problem.
Another concern is PromptUserToSaveOrUpdatePassword which takes a pointer now. Apparently, we'll have to move it a few times before the object lands to where it belongs. PasswordManager::ProvisionallySavePassword erases an object..
Third concern (theoretical one) is that PasswordFormManager seems to be big enough and sometimes there is no consecutive block of memory available. I don't think it happens in practice.
As we allocate, we hold a mostly stable array of PasswordFormManager for some time and then clear it (but not destruct). We probably would improve performance as ProvisionallySavePassword happens seldom but it worth measuring :)

Thanks for the great answer, Vasilii!

I agree, making PasswordForm movable is a good first step.

Assuming PasswordForm is movable, what is the problem with observed_form_?

Splitting provisional_save_manager_ in a unique_ptr and passing it as such over the various places it needs to go through could be kept as is. Since it is a single object, passing it by value does not seem to have any advantages.

The concern about consecutive memory is perhaps something to watch for on mobile devices, although at the point when allocating a couple of kilobytes is a problem, Chrome is unlikely to work for too long anyway.

Measuring performance in general is something I should think about more. Perhaps I should do that first and then revisit this.

observed_form_ is const. I don't know how to move the data out of const variable.
To measure performance we need to implement something first. Maybe a test. I think we'll gain something.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Actually putting the PasswordForm suggestion from #3 in the title. It seems the most impactful.

The caveat is that we might be splitting PasswordForm into smaller classes once the new form parser becomes the only parser (among other things, PasswordForm will only be used in browser process from that point on). So making PasswordForm movable after that might be less effort.

In general, the peformance seems to be a low concern for password manager code (much less than, say, flaky tests, correctness bugs, etc.). So investing here might not be the most efficient. I was considering closing this as WontFix for that reason, but OTOH, if we need a deep-dive-starter project, this might be a candidate.

But PasswordForm is already movable for a long time.

I see now: https://crrev.com/c/678496

Thanks for pointing this out, Vasilii!

Also, sorry for forgetting to update the title.

I don't think it makes much sense to have a bug open for the rest. If we realise that we do need to start moving PFM or the fetcher around, we'll likely just implement it. The existence of this bug just adds clutter, so I'm closing it. If you disagree, feel free to reopen.