You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2015/09/02 18:46:04 UTC
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-couch-epi/pull/8
2796 improve performance of providers function
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-couch-epi 2796-improve_performance_of_providers_function
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch-epi/pull/8.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #8
----
commit 5e0f14b4b9e1ab79bbc1d3ffbc27d35a551bfc43
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-09-02T14:07:19Z
Fix test suite
commit 76a7b1bbaf09fa47d02687d323b6a2871f2f7918
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-09-02T15:10:52Z
Introduce 'couch_epi:register_service/1'
commit 3b99646720d183828c6eebfa216425f8546c291e
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-09-02T15:11:36Z
Call 'maybe_start_keeper' for 'couch_epi_data_source'
commit 98602eba030608b2616e3ed500f6901f9e03a665
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-09-02T16:37:46Z
Don't use try/catch to handle missing plugins
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137181604
Since we have only two places in two modules which we need to care about. I could check for `erlang:exported_function` in place to avoid confusion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#discussion_r38560507
--- Diff: src/couch_epi_data_gen.erl ---
@@ -174,27 +175,43 @@ module_name({Service, Key}) when is_list(Service) andalso is_list(Key) ->
is_updated(Handle, Source, Data) ->
Sig = couch_epi_util:hash(Data),
- try Handle:version(Source) of
- {error, {unknown, Source}} -> true;
- {error, Reason} -> throw(Reason);
- Sig -> false;
- _ -> true
- catch
- error:undef -> true;
- Class:Reason ->
- throw({Class, {Source, Reason}})
- end.
-
+ if_exists(Handle, version, 1, true, fun() ->
+ try Handle:version(Source) of
+ {error, {unknown, Source}} -> true;
+ {error, Reason} -> throw(Reason);
+ Sig -> false;
+ _ -> true
+ catch
+ Class:Reason ->
+ throw({Class, {Source, Reason}})
+ end
+ end).
+
+save(Handle, undefined, []) ->
+ case current_data(Handle) of
+ [] -> generate(Handle, []);
+ _Else -> ok
+ end;
save(Handle, Source, Data) ->
- CurrentData = get_current_data(Handle),
+ CurrentData = current_data(Handle),
NewDefs = lists:keystore(Source, 1, CurrentData, {Source, Data}),
generate(Handle, NewDefs).
-get_current_data(Handle) ->
- try Handle:by_source()
- catch error:undef -> []
- end.
+current_data(Handle, Subscriber) ->
+ if_exists(Handle, by_source, 1, [], fun() ->
+ Handle:by_source(Subscriber)
+ end).
+current_data(Handle) ->
+ if_exists(Handle, by_source, 0, [], fun() ->
+ Handle:by_source()
+ end).
+
+if_exists(Handle, Func, Arity, Default, Fun) ->
--- End diff --
`A` is the length of arguments list, so you can calculate it with easy.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#discussion_r38559864
--- Diff: src/couch_epi_data_gen.erl ---
@@ -174,27 +175,43 @@ module_name({Service, Key}) when is_list(Service) andalso is_list(Key) ->
is_updated(Handle, Source, Data) ->
Sig = couch_epi_util:hash(Data),
- try Handle:version(Source) of
- {error, {unknown, Source}} -> true;
- {error, Reason} -> throw(Reason);
- Sig -> false;
- _ -> true
- catch
- error:undef -> true;
- Class:Reason ->
- throw({Class, {Source, Reason}})
- end.
-
+ if_exists(Handle, version, 1, true, fun() ->
+ try Handle:version(Source) of
+ {error, {unknown, Source}} -> true;
+ {error, Reason} -> throw(Reason);
+ Sig -> false;
+ _ -> true
+ catch
+ Class:Reason ->
+ throw({Class, {Source, Reason}})
+ end
+ end).
+
+save(Handle, undefined, []) ->
+ case current_data(Handle) of
+ [] -> generate(Handle, []);
+ _Else -> ok
+ end;
save(Handle, Source, Data) ->
- CurrentData = get_current_data(Handle),
+ CurrentData = current_data(Handle),
NewDefs = lists:keystore(Source, 1, CurrentData, {Source, Data}),
generate(Handle, NewDefs).
-get_current_data(Handle) ->
- try Handle:by_source()
- catch error:undef -> []
- end.
+current_data(Handle, Subscriber) ->
+ if_exists(Handle, by_source, 1, [], fun() ->
+ Handle:by_source(Subscriber)
+ end).
+current_data(Handle) ->
+ if_exists(Handle, by_source, 0, [], fun() ->
+ Handle:by_source()
+ end).
+
+if_exists(Handle, Func, Arity, Default, Fun) ->
--- End diff --
`if_exists` suppose to call a function with passed arguments. If we want to rename `if_exists` into `maybe_call` we would need to use `apply(M, F, A)`. Since we have only two kinds of functions (with arity 0 and arity 1). We could have maybe_call with different arity . Something like:
```erlang
maybe_call(Handle, Function, Default) ->
case erlang:function_exported(Handle, Func, 0) of
true -> Handle:Function();
false -> Default
end.
maybe_call(Handle, Function, Arg, Default) ->
case erlang:function_exported(Handle, Func, 1) of
true -> Handle:Function(Arg);
false -> Default
end.
```
This approach wouldn't work for this https://github.com/cloudant/couchdb-couch-epi/blob/36b2be55a89f766df44b05d988d040e6a3125529/src/couch_epi_data_gen.erl#L178. Since we need to use try/catch to detect other errors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137177377
ok, reviewed, I think kxepal and I both found the implementation and name of `is_exists` to be a little odd, what do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#discussion_r38556013
--- Diff: src/couch_epi_data_gen.erl ---
@@ -174,27 +175,43 @@ module_name({Service, Key}) when is_list(Service) andalso is_list(Key) ->
is_updated(Handle, Source, Data) ->
Sig = couch_epi_util:hash(Data),
- try Handle:version(Source) of
- {error, {unknown, Source}} -> true;
- {error, Reason} -> throw(Reason);
- Sig -> false;
- _ -> true
- catch
- error:undef -> true;
- Class:Reason ->
- throw({Class, {Source, Reason}})
- end.
-
+ if_exists(Handle, version, 1, true, fun() ->
+ try Handle:version(Source) of
+ {error, {unknown, Source}} -> true;
+ {error, Reason} -> throw(Reason);
+ Sig -> false;
+ _ -> true
+ catch
+ Class:Reason ->
+ throw({Class, {Source, Reason}})
+ end
+ end).
+
+save(Handle, undefined, []) ->
+ case current_data(Handle) of
+ [] -> generate(Handle, []);
+ _Else -> ok
+ end;
save(Handle, Source, Data) ->
- CurrentData = get_current_data(Handle),
+ CurrentData = current_data(Handle),
NewDefs = lists:keystore(Source, 1, CurrentData, {Source, Data}),
generate(Handle, NewDefs).
-get_current_data(Handle) ->
- try Handle:by_source()
- catch error:undef -> []
- end.
+current_data(Handle, Subscriber) ->
+ if_exists(Handle, by_source, 1, [], fun() ->
+ Handle:by_source(Subscriber)
+ end).
+current_data(Handle) ->
+ if_exists(Handle, by_source, 0, [], fun() ->
+ Handle:by_source()
+ end).
+
+if_exists(Handle, Func, Arity, Default, Fun) ->
--- End diff --
`maybe_call` sounds as better name for this and less confusing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137170900
Even though earlier solution with usage of `erlang:function_exported` improves performance. We have to pay the cost of `erlang:function_exported` on every call to `couch_epi:apply`. With this PR we would pay a little upfront on `couch_epi:register_service`. Without any penalty when doing `couch_epi:apply`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137169246
we discussed this and agree checking function_exported would solve the performance problem. So why now is there a completely different (and large) change to review?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137181968
PR also needs a description and a proper title.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137174664
List of related PRs:
- https://github.com/apache/couchdb-global-changes/pull/11
- https://github.com/apache/couchdb-chttpd/pull/67
- https://github.com/apache/couchdb-couch/pull/94
- https://github.com/apache/couchdb-couch-index/pull/8
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-couch-epi/pull/8
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137190733
I guess it's the cleanest we can do. +1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#discussion_r38556048
--- Diff: src/couch_epi_data_gen.erl ---
@@ -174,27 +175,43 @@ module_name({Service, Key}) when is_list(Service) andalso is_list(Key) ->
is_updated(Handle, Source, Data) ->
Sig = couch_epi_util:hash(Data),
- try Handle:version(Source) of
- {error, {unknown, Source}} -> true;
- {error, Reason} -> throw(Reason);
- Sig -> false;
- _ -> true
- catch
- error:undef -> true;
- Class:Reason ->
- throw({Class, {Source, Reason}})
- end.
-
+ if_exists(Handle, version, 1, true, fun() ->
+ try Handle:version(Source) of
+ {error, {unknown, Source}} -> true;
+ {error, Reason} -> throw(Reason);
+ Sig -> false;
+ _ -> true
+ catch
+ Class:Reason ->
+ throw({Class, {Source, Reason}})
+ end
+ end).
+
+save(Handle, undefined, []) ->
+ case current_data(Handle) of
+ [] -> generate(Handle, []);
+ _Else -> ok
+ end;
save(Handle, Source, Data) ->
- CurrentData = get_current_data(Handle),
+ CurrentData = current_data(Handle),
NewDefs = lists:keystore(Source, 1, CurrentData, {Source, Data}),
generate(Handle, NewDefs).
-get_current_data(Handle) ->
- try Handle:by_source()
- catch error:undef -> []
- end.
+current_data(Handle, Subscriber) ->
+ if_exists(Handle, by_source, 1, [], fun() ->
+ Handle:by_source(Subscriber)
+ end).
+current_data(Handle) ->
+ if_exists(Handle, by_source, 0, [], fun() ->
+ Handle:by_source()
+ end).
+
+if_exists(Handle, Func, Arity, Default, Fun) ->
--- End diff --
Also, why do you pass closure instead of simple apply call?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137181011
@rnewson: `Module:Fun(Args)` wouldn't work. We would have to handle `Module:Fun(Arg)` and `Module:Fun()`.
With `eval_if_exported` approach I would have to do:
```
try eval_if_exported(Handle, version, Source) of
{error, {unknown, Source}} -> true;
{error, Reason} -> throw(Reason);
Sig -> false;
_ -> true
catch
Class:Reason ->
throw({Class, {Source, Reason}})
end
end).
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-epi pull request: 2796 improve performance of provid...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-epi/pull/8#issuecomment-137172045
hrm, ok. will review shortly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---