Comment on attachment 8572820 [details] [diff] [review] patch Interesting... This certainly looks like it saves some cycles when creating accessibles! r=me since I didn't see anything missing from the old way. :)

Comment on attachment 8572820 [details] [diff] [review] patch I'm kind of dubious this is any faster, hash tables are kind of slow and hard on caches. Its also silly to use the address of the atom when they contain a hash of the actual string. I think if you actually want this to be fast the best thing is probably to switch on the first char of the incoming tag name, but I'd probably want to see numbers for any attempt where the goal is speed. >+Accessible* >+New_HTMLLink(nsIContent* aContent, Accessible* aContext) these all really need to be static. >+ >+#define MARKUPMAP(atom, new_func) \ >+ { &nsGkAtoms::atom, new_func }, >+ >+static const MarkupMapInfo sMarkupMapList[] = { >+ #include "MarkupMap.h" >+}; >+ >+#undef MARKUPMAP > > static const char16_t kInitIndicator[] = { '1', 0 }; > observerService->NotifyObservers(nullptr, "a11y-init-or-shutdown", kInitIndicator); > >+ for (uint32_t i = 0; i < ArrayLength(sMarkupMapList); i++) >+ mMarkupMap.Put(*sMarkupMapList[i].tag, sMarkupMapList[i].new_func); this store array of data and then initialize a hash table with it thing is really silly, you might as well just use the macros to initialize the hash table.

(In reply to Trevor Saunders (:tbsaunde) from comment #3) > Comment on attachment 8572820 [details] [diff] [review] > patch > > I'm kind of dubious this is any faster, hash tables are kind of slow and > hard on caches. can you develop that point? how does it happen that hash table is no way faster than sequence of ifs. > >+Accessible* > >+New_HTMLLink(nsIContent* aContent, Accessible* aContext) > > these all really need to be static. thanks, I'll file follow up on it. > > static const char16_t kInitIndicator[] = { '1', 0 }; > > observerService->NotifyObservers(nullptr, "a11y-init-or-shutdown", kInitIndicator); > > > >+ for (uint32_t i = 0; i < ArrayLength(sMarkupMapList); i++) > >+ mMarkupMap.Put(*sMarkupMapList[i].tag, sMarkupMapList[i].new_func); > > this store array of data and then initialize a hash table with it thing is > really silly, you might as well just use the macros to initialize the hash > table. can you file bug pls?

(In reply to alexander :surkov from comment #4) > (In reply to Trevor Saunders (:tbsaunde) from comment #3) > > Comment on attachment 8572820 [details] [diff] [review] > > patch > > > > I'm kind of dubious this is any faster, hash tables are kind of slow and > > hard on caches. > > can you develop that point? how does it happen that hash table is no way > faster than sequence of ifs. how do you think hash table lookup is implemented that it isn't also also a bunch of code with some number of branches? My guess is its fewer branches but worse locality, but that's only a guess, and I have no clue how the numbers will work out.

(In reply to Trevor Saunders (:tbsaunde) from comment #5) > (In reply to alexander :surkov from comment #4) > > (In reply to Trevor Saunders (:tbsaunde) from comment #3) > > > Comment on attachment 8572820 [details] [diff] [review] > > > patch > > > > > > I'm kind of dubious this is any faster, hash tables are kind of slow and > > > hard on caches. > > > > can you develop that point? how does it happen that hash table is no way > > faster than sequence of ifs. > > how do you think hash table lookup is implemented that it isn't also also a > bunch of code with some number of branches? My guess is its fewer branches > but worse locality, but that's only a guess, and I have no clue how the > numbers will work out. right, we increased the number by mathml support and the tendency is to add new tags, so as long as we are not slower then it's reasonable to have it.

(In reply to alexander :surkov from comment #6) > (In reply to Trevor Saunders (:tbsaunde) from comment #5) > > (In reply to alexander :surkov from comment #4) > > > (In reply to Trevor Saunders (:tbsaunde) from comment #3) > > > > Comment on attachment 8572820 [details] [diff] [review] > > > > patch > > > > > > > > I'm kind of dubious this is any faster, hash tables are kind of slow and > > > > hard on caches. > > > > > > can you develop that point? how does it happen that hash table is no way > > > faster than sequence of ifs. > > > > how do you think hash table lookup is implemented that it isn't also also a > > bunch of code with some number of branches? My guess is its fewer branches > > but worse locality, but that's only a guess, and I have no clue how the > > numbers will work out. > > right, we increased the number by mathml support and the tendency is to add > new tags, so as long as we are not slower then it's reasonable to have it. uh, maybe, that's making assumptions about where the cut over is and how quickly things grow. Doing something eventually depending on it being a problem may make sense, but I'm not convinced this is a particularly great approach.

there's always room for improvements, you're always welcome to come with ideas/opinion/help.

(In reply to alexander :surkov from comment #8) > there's always room for improvements, you're always welcome to come with > ideas/opinion/help. I think what I was doing is known as giving an opinion. I'd suggest git revert and then work on a problem we have evidence is important.

(In reply to Trevor Saunders (:tbsaunde) from comment #9) > (In reply to alexander :surkov from comment #8) > > there's always room for improvements, you're always welcome to come with > > ideas/opinion/help. > > I think what I was doing is known as giving an opinion. I'd suggest git > revert and then work on a problem we have evidence is important. It's not an option I would consider, I'd welcome more constructive ideas.

I should have raised this issue during the 'controversial changes' discussion :-) As I said to Alex in Toronto, another issue with hash tables is that they consume more memory than needed since the table must be large enough to reduce the probability of collisions. In other parts of the Gecko code, we generally just use sorted arrays and a binary search to find one element.

Memory shouldn't be issue here since tag number we use is relatively small. Anyway, we have bug 1143101 for improvements, it's worth to move the discussion there. Btw, you should feel free to take it ;)