Change existing symbol_namespace::iterate to return all data instead of invoking a callback #3072

hkaiser · 2017-12-12T02:29:03Z

This API was never used anywhere and required some face-lifting after being half-way abandoned. It is now needed on one of our projects.

…th AGAS - this depends on STEllAR-GROUP/hpx#3072

sithhell · 2017-12-12T07:56:22Z

src/runtime/agas/server/symbol_namespace_server.cpp

+// Reuse functionaliy from perf-counters
+namespace hpx { namespace performance_counters { namespace detail
+{
+    HPX_EXPORT std::string regex_from_pattern(std::string const& pattern,


Wouldn't it make sense to move this into hpx::util?

Yah, may make sense to do that - but that's unrelated to the current PR.

Someone once told me that nothing is more permanent than a temporary solution ;)

I have not added any temporary solutions. I used what was there for years already.

What I meant is the temporary solution of pulling in the performance counters here for a utility function that is in itself unrelated to both performance counters and symbol namespace.

sithhell · 2017-12-12T07:57:57Z

src/runtime/agas/addressing_service.cpp

-            return ids;
-        }
-    }
-
 /// Invoke the supplied hpx::function for every registered global name


This comment should be fixed. There is no function involved there anymore.

Good catch, will fix.

…th AGAS - this depends on STEllAR-GROUP/hpx#3072 - add an example demonstrating adding and accessing debug information to primitive instances

hkaiser · 2017-12-13T01:18:42Z

@sithhell could we go ahead with this, please?

…of invoking a callback

…th AGAS - this depends on STEllAR-GROUP/hpx#3072 - add an example demonstrating adding and accessing debug information to primitive instances

sithhell

In principle the code is ok. After thinking a while about it, I'm sceptical about scalability since all symbols are now collected into a single place. This sounds like quite some overhead for just iterating the registered names.
When looking at the use case in phylanx, I think this might be solved more efficiently by calculating the symbolic names from the ast, and use those to resolve the gids. This has the additional advantage that the number of localities to broadcast to is reduced.

sithhell · 2017-12-13T08:39:56Z

src/runtime/agas/server/symbol_namespace_server.cpp

+            if (!boost::regex_match(it->first, rx))
+                continue;
+            found[it->first] =
+                naming::detail::split_gid_if_needed(*(it->second)).get();


The lock needs to be unlocked here due to a potential remote operation.

Good point.

sithhell · 2017-12-13T08:41:27Z

src/runtime/agas/server/symbol_namespace_server.cpp

+            if (!pattern.empty() && pattern != it->first)
+                continue;
+            found[it->first] =
+                naming::detail::split_gid_if_needed(*(it->second)).get();


hkaiser · 2017-12-13T12:27:53Z

In principle the code is ok. After thinking a while about it, I'm sceptical about scalability since all symbols are now collected into a single place. This sounds like quite some overhead for just iterating the registered names.

Well, if you need all symbols in one place you should collect them there, no?

When looking at the use case in phylanx, I think this might be solved more efficiently by calculating the symbolic names from the ast, and use those to resolve the gids. This has the additional advantage that the number of localities to broadcast to is reduced.

Could you elaborate? I don't think I understand what you're after.

sithhell · 2017-12-13T13:44:26Z

In principle the code is ok. After thinking a while about it, I'm sceptical about scalability since all symbols are now collected into a single place. This sounds like quite some overhead for just iterating the registered names.

Well, if you need all symbols in one place you should collect them there, no?

Sure, what I am questioning though is if you really need all in one place.

When looking at the use case in phylanx, I think this might be solved more efficiently by calculating the symbolic names from the ast, and use those to resolve the gids. This has the additional advantage that the number of localities to broadcast to is reduced.

Could you elaborate? I don't think I understand what you're after.

The parser is creating the AST nodes with specific names which are then registered in the symbolic namespace. Giving that you have the AST, it should be trivial to extract the names of the different nodes by a simple tree traversal. Once you have a list of the names, you can calculate which service instance is responsible for the each name and send out bulk requests to only those instances of the symbolic namespace server to resolve the GID. That is, you shouldn't need the symbolic namespace service to get all names associated with the AST.

sithhell · 2017-12-13T19:46:19Z

hpx/util/regex_from_pattern.hpp

+
+namespace hpx { namespace util
+{
+    HPX_EXPORT std::string regex_from_pattern(std::string const& pattern,


Thanks! Much appreciated!

sithhell · 2017-12-13T19:53:55Z

src/runtime/agas/server/symbol_namespace_server.cpp

+    // now, split gids, if needed
+    for (auto& e : found)
+    {
+        e.second = naming::detail::split_gid_if_needed(e.second).get();


I am not sure if that is correct. The above lines (394 and 405) copy the gid_type. This means that we also copy the credit, that means that we always split from the same initial credit count, without asking for more credit in the primary namespace. This will lead to a negative credit count when all credits are returned back to AGAS.

Fair point, not sure what I was thinking :/

sithhell · 2017-12-14T18:24:03Z

src/runtime/agas/server/symbol_namespace_server.cpp

-            f(key, gid);
+
+            found[it->first] =
+                naming::detail::split_gid_if_needed(*current_gid).get();


Yeah, this will work. While going through the PR I was really wondering why we don't reuse hpx::naming::id_type here instead of std::shared_ptr<gid_type>. But that's unrelated to that PR.

AGAS stores gid_type instances only by design. For all intents and purposes this is sufficient and avoids problems with reference counting etc. The symbol namespace is special in that regard that it actually has to maintain reference counts (for some of its functionality). I'm impartial to changing this - it wouldn't make the code so much more readable, however.

sithhell

LGTM now.
I'm still skeptical though if this is really a better approach than the callback based solution.

hkaiser · 2017-12-14T18:47:12Z

I'm still skeptical though if this is really a better approach than the callback based solution.

I'd be happy to add back the old callback-based solution as an alternate API. For my use case it would be much more expensive at runtime to use the old API, however.

…th AGAS - this depends on STEllAR-GROUP/hpx#3072 - add an example demonstrating adding and accessing debug information to primitive instances

hkaiser added category: AGAS type: enhancement labels Dec 12, 2017

hkaiser added this to the 1.1.0 milestone Dec 12, 2017

hkaiser added a commit to STEllAR-GROUP/phylanx that referenced this pull request Dec 12, 2017

Adding unique names to primitives and register primitive instances wi…

b550830

…th AGAS - this depends on STEllAR-GROUP/hpx#3072

hkaiser added a commit to STEllAR-GROUP/phylanx that referenced this pull request Dec 12, 2017

Adding unique names to primitives and register primitive instances wi…

7baf873

…th AGAS - this depends on STEllAR-GROUP/hpx#3072

sithhell reviewed Dec 12, 2017

View reviewed changes

hkaiser force-pushed the find_symbols branch from 0ea2ea3 to 5219aff Compare December 12, 2017 15:32

hkaiser mentioned this pull request Dec 12, 2017

Adding unique names to primitives and register primitive instances with AGAS STEllAR-GROUP/phylanx#126

Merged

Change existing symbol_namespace::iterate to return all data instead …

c38c6dc

…of invoking a callback

hkaiser force-pushed the find_symbols branch from 5219aff to c38c6dc Compare December 13, 2017 01:20

sithhell requested changes Dec 13, 2017

View reviewed changes

sithhell reviewed Dec 13, 2017

View reviewed changes

Addressing review comments

fc4482b

hkaiser force-pushed the find_symbols branch from a9e0248 to fc4482b Compare December 14, 2017 00:40

sithhell reviewed Dec 14, 2017

View reviewed changes

sithhell approved these changes Dec 14, 2017

View reviewed changes

hkaiser merged commit c76e60a into master Dec 14, 2017

hkaiser deleted the find_symbols branch December 14, 2017 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change existing symbol_namespace::iterate to return all data instead of invoking a callback #3072

Change existing symbol_namespace::iterate to return all data instead of invoking a callback #3072

hkaiser commented Dec 12, 2017

sithhell Dec 12, 2017

hkaiser Dec 12, 2017

sithhell Dec 13, 2017

hkaiser Dec 13, 2017

sithhell Dec 13, 2017

sithhell Dec 12, 2017

hkaiser Dec 12, 2017

hkaiser commented Dec 13, 2017

sithhell left a comment

sithhell Dec 13, 2017

hkaiser Dec 13, 2017

sithhell Dec 13, 2017

hkaiser commented Dec 13, 2017

sithhell commented Dec 13, 2017

sithhell Dec 13, 2017

sithhell Dec 13, 2017

hkaiser Dec 13, 2017

sithhell Dec 14, 2017

hkaiser Dec 14, 2017

sithhell left a comment

hkaiser commented Dec 14, 2017

Change existing symbol_namespace::iterate to return all data instead of invoking a callback #3072

Change existing symbol_namespace::iterate to return all data instead of invoking a callback #3072

Conversation

hkaiser commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaiser commented Dec 13, 2017

sithhell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaiser commented Dec 13, 2017

sithhell commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sithhell left a comment

Choose a reason for hiding this comment

hkaiser commented Dec 14, 2017