Closed Bug 681891 Opened 10 years ago Closed 9 years ago

GCLI Field.claim should be optimized

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P4)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: jwalker, Unassigned)

Details

(Whiteboard: [best:1d, likely:1d, worst:1d])

The whole Field.claim stuff seems like there could be a better/more performant way. I like the abstraction, but I'm not sure about the implementation. How many different types of fields do we expect there to be? Would it make sense to use an algorithm that isn't O(n)? Instead of checking everytime, could we just look this info up in an object as if it was a hash table? This would require the types to have unique string representations, which I'm not sure whether or not they do (feel free to correct me here).

If Field.claim were replaced by the imaginary claimField, the code might look something like this:

    function StringField(...) {
      ...
    }
    StringField.prototype = Object.create(Field.prototype);
    
    claimField(Field.MATCH, StringType.toString(), StringField);
    claimField(Field.IF_NOTHING_BETTER, BooleanType.toString(), StringField);

Not only would this move the algorithm from O(n) -> O(1), but it would make the calculations for which field is the best fit for a given type happen only when new fields are claimed, not every time we are looking for the right field.

Then getField and friends would need to be updated a little as well:

    var fieldCtors = {};
    
    function claimField(level, typeString, Ctor) {
      if (!fieldCtors[typeString] || level > fieldCtors[typeString].level) {
        fieldCtors[typeString] = { level: level, ctor: Ctor };
      }
    }
    
    function getField(...) {
      ...
      var typeString = type.constructor.toString();
      if (typeString in fieldCtors) {
        return new fieldCtors[typeString].ctor(...)
      } else if ('DEFAULT_FIELD' in fieldCtors) {
        return fieldCtors.DEFAULT_FIELD.ctor(...)
      } else {
        throw new Error(...)
      }
    }
    
    ...

It definitely means that less will have to be computed after initialization, and less redundant computations.
I agree that this is probably a good idea, and I'm particularly pleased that you picked up on it because I didn't like it.

However I have 2 reservations:
- It seems to me not inconcievable that we might want to select different fields based on more than just the type of the argument. If we have a selection with less than 5 options then maybe we'd use a set of <input type=radio> rather than a <select>, for example.
- This could be a premature optimization

On the other hand:
- I don't like the current implemenation
- The flexability might not be a good idea anyway

So I've made it block GCLI-HELP which is the pot for 'nice to have' for the next release.
Assignee: nobody → jwalker
Moving to GCLI-FUTURE because we've done enough of the bugs in GCLI-HELP that it's not really a 'nice to have' list.
Blocks: GCLI-FUTURE
No longer blocks: GCLI-HELP
Assignee: jwalker → nobody
OS: Windows 7 → All
Hardware: x86_64 → All
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Triage. Filter on PEGASUS.
Priority: -- → P4
Triage. Filter on PEGASUS.
No longer blocks: GCLI-FUTURE
New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"
Component: Developer Tools: Console → Developer Tools: Graphic Commandline and Toolbar
I'm not sure having trivial code enhancements bugs like this on file helps. They slow down triage, and practically we'll fix them when the become a problem.

aka: http://www.joelonsoftware.com/items/2012/07/09.html
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.