-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stdlib: Make vertex and edge ids in digraph unique #9361
stdlib: Make vertex and edge ids in digraph unique #9361
Conversation
CT Test Results 2 files 96 suites 1h 8m 40s ⏱️ Results for commit 1727aa8. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
3e8da4b
to
1727aa8
Compare
@@ -153,6 +153,8 @@ subgraph(Config) when is_list(Config) -> | |||
{fg,f,g,fgl} = digraph:edge(SG, fg), | |||
{fg2,f,g,fgl2} = digraph:edge(SG, fg2), | |||
{_, {_, acyclic}} = lists:keysearch(cyclicity, 1, digraph:info(SG)), | |||
digraph:add_edge(SG, f, g), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't fail if I remove the bug fix. You will need to test the return value.
@@ -153,6 +153,8 @@ subgraph(Config) when is_list(Config) -> | |||
{fg,f,g,fgl} = digraph:edge(SG, fg), | |||
{fg2,f,g,fgl2} = digraph:edge(SG, fg2), | |||
{_, {_, acyclic}} = lists:keysearch(cyclicity, 1, digraph:info(SG)), | |||
digraph:add_edge(SG, f, g), | |||
digraph:add_vertex(SG, b), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cannot fail because you are specifying the name of the new vertex. You will need to call digraph:add_vertex(G)
and check the return value. The graph will also have to contain some vertices created with default names.
true = ets:insert(NT, {'$eid', K+1}), | ||
['$e' | K]. | ||
new_edge_id() -> | ||
['$e' | erlang:unique_integer()]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
erlang:unique_integer/0
will usually return a negative integer. The documentation says that the integer is greater than or equal to zero.
This way of solving the problem works fine on a 64-bit system. In practice, all integers will always be small integers (fitting in a word).
However, on a 32-bit system, the integers from erlang:unique_integer/0
can start to become bignums that no longer fit in a word. That could degrade performance in a long-running system.
So I think it would be better to not change how the integers are generated in digraph
, but instead change digraph_utils:subgraph()
to ensure that the $eid
and $vid
values are copied into the new digraph. One way to implement that would be to add a new function digraph:new(G, Type)
, which would create a new digraph and inherit the $eid
and $vid
values from G
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, setting $eid
and $vid
according to the number of vertices and edges in a subgraph would be another way to fix it. I'll make an internal function to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, on a 32-bit system, the integers from
erlang:unique_integer/0
can start to become bignums that no longer fit in a word. That could degrade performance in a long-running system.
Certainly, performance will degrade (slightly) once the line is crossed, but erlang:unique_integer([positive])
will still be much faster than the current dance in ETS:
new_edge_id(G) ->
NT = G#digraph.ntab,
[{'$eid', K}] = ets:lookup(NT, '$eid'),
true = ets:delete(NT, '$eid'),
true = ets:insert(NT, {'$eid', K+1}),
['$e' | K].
We can also store a "baseline" number that is the erlang:unique_integer()
when the graph was first created, and make all edge identifiers relative to that, eating the bignum cost only once per edge unless a graph becomes too long-lived.
Another variant is to use the counters
module.
So I think it would be better to not change how the integers are generated in digraph, but instead change digraph_utils:subgraph() to ensure that the $eid and $vid values are copied into the new digraph. One way to implement that would be to add a new function digraph:new(G, Type), which would create a new digraph and inherit the $eid and $vid values from G.
Would this be exposed to the user? I would expect digraph:new/2
taking an existing graph to make a copy thereof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly, performance will degrade (slightly) once the line is crossed, but
erlang:unique_integer([positive])
will still be much faster than the current dance in ETS
Yes, but performance could be degraded for other parts of the system that use erlang:unique_integer/1
, because it is a global resource.
I'll make another PR with the new fix. This PR should target maint with a different branch name. Closing it now. |
Fix #9191