Avoid trapping exceptions by using reflection #59
Conversation
|
Thanks for the PR. IT looks like travis does not like the formatting. Never the less, have a look at my comment. |
|
I do not see a code comment. I'll try using a different version of dartfmt and see if it likes it; this was dartfmt-ed with 1.24.0-dev.3.0. |
lib/mustache_context.dart
Outdated
| //Try to make dart2js understand that when we define USE_MIRRORS = false | ||
| //we do not want to use any reflector | ||
| return (useMirrors && USE_MIRRORS) ? ctxReflector[key] : null; | ||
| if (reflect(ctx).type.instanceMembers.containsKey(new Symbol("[]"))) { |
There was a problem hiding this comment.
This check should be done if and only if (useMirrors && USE_MIRRORS) otherwise the code will get transpiled to js with a lot more code, unless something changed on the dart side.
There was a problem hiding this comment.
What do you mean by done? You still use reflect without a check on useMirrors.
There was a problem hiding this comment.
Sorry, was reviewing wrong commit :). It looks good now. I'll try to bake a new version tonight.
|
I believe I've addressed the comment; dartfmt error was due to a change in formatting (unrelated to my change) with dart-style 1.0.5. |
| if (reflectedString.isAssignableTo(m.parameters[0].type)) { | ||
| try { | ||
| return ctx[key]; | ||
| } catch (NoSuchMethodError) { |
There was a problem hiding this comment.
I believe, here a null should be returned. I am not sure though, I have to check.
There was a problem hiding this comment.
Previously, if (useMirrors && USE_MIRRORS) is true and the attempt to use ctx[key] resulted in an exception, we returned ctxReflector[key]. So a noop here falls through to the base case of returning ctxReflector[key]. The other two if statements just try to get us to return ctxReflector[key] if we can predict in advance that calling ctx[key] will raise an exception.
|
Hi @valotas , just making sure there's nothing else you need from me. |
|
@jcollins-g all good. Just released |
| if (reflect(ctx).type.instanceMembers.containsKey(new Symbol("[]"))) { | ||
| MethodMirror m = reflect(ctx).type.instanceMembers[new Symbol("[]")]; | ||
| TypeMirror reflectedString = reflectType(String); | ||
| if (reflectedString.isAssignableTo(m.parameters[0].type)) { |
There was a problem hiding this comment.
It looks like dart2js has some problems with this one:
00:26 +8 -10: Deep subcontext test [E]
UnimplementedError
dart:_internal dart<.JsTypeMirror.isAssignableTo
package:mustache4dart/mustache_context.dart 257:9 dart<._ObjectReflector.fieldValue
package:mustache4dart/mustache_context.dart 241:26 dart<._ObjectReflector.$index
package:mustache4dart/mustache_context.dart 123:14 dart<._MustacheContext._getActualValue
package:mustache4dart/mustache_context.dart 100:13 dart<._MustacheContext._getContextForKey
package:mustache4dart/mustache_context.dart 69:18 dart<._MustacheContext._getInThisOrParent
package:mustache4dart/mustache_context.dart 65:12 dart<._MustacheContext.field
test/mustache_context_test.dart 192:12 dart<.main.<fn>
I'm afraid I have to trap some exception after all. Do you mind sharing your usecase? Maybe I can try the exception trapping deeper in order not to hit the performance .
There was a problem hiding this comment.
That exception wouldn't be trapped in any case, because UnimplementedError is not a subclass of NoSuchMethodError. So it seems unlikely that reverting this change would correct that test. Does it?
There was a problem hiding this comment.
Oh, never mind... isAssignableTo isn't implemented. Ouch.
Well, trapping the exception isn't the problem, it is catching it that impacts performance in the vm. As long as catching it doesn't introduce a performance problem in dart2js, it should be safe to do that from my case's perspective.
Currently, dartdoc makes extensive use of the exception trapping and reflection inside mustache4dart. But trapping exceptions is expensive and profiling suggests a great deal of time is spent in here. This should remove the hotspot for clients like dartdoc without altering mustache4dart's behavior.