600754 - Do not add and remove form controls from their form when their type changes

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Mounir Lamouri (:mounir)]

When a nsGenericHTMLFormElement has its type changing, it is removed from it's form on BeforeSetAttr and added back on AfterSetAttr. This behavior is clearly nicer considering some checks have to be done but in most situations, it's useless and setting the type might happen often (and even more with new input types).
For example, each time the type is changed, we have to found the new position of the element in the list which is only needed if the element's type change from a listed one to a non-listed one (can that happen except with 'image'?).

I've wrote a small test that is checking how long we spend adding input to a form if we set the type each time. Not calling AddElement/RemoveElement saves ~45% on a debug build. This might be smaller on release considering there is some special code only run in debug in AddElement.

Even if we can save some time, I guess that is not really important considering adding form controls to a form isn't a very expensive operation?

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: perf check

Comment 2

[Mounir Lamouri (:mounir)]

I've check in a release build and the initial numbers are really smaller so I guess this should be a good thing to do because what we currently do is quite dumb but there will be no big performance win in real usage.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #0)
> Not calling AddElement/RemoveElement saves
> ~45% on a debug build.

Note, usually it makes no sense to run performance tests on a debug build.
We run a lot more code there.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> what we currently do is quite dumb

Well, except for the "it's simple and shares other existing code" part, right?

As long as we keep those two properties...

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to comment #4)
> > what we currently do is quite dumb
> 
> Well, except for the "it's simple and shares other existing code" part, right?
> 
> As long as we keep those two properties...

The think is I've an assert that happens because of that code (we do something when an element is added and removed from a form) so I had to introduce a boolean to not run this code when we remove/add an element because of a type change.
But I agree that's not something important.

(In reply to comment #3)
> Note, usually it makes no sense to run performance tests on a debug build.
> We run a lot more code there.

I knew but I only have debug builds. I should probably have a repository with a release build for that kind of things...

Comment 6

[Mounir Lamouri (:mounir)]

See bug 596785

Comment 7

[Boris Zbarsky [:bzbarsky]]

You can build both debug and release from the same repository; just use different objdirs.

Comment 8

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.