Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

stdlib: Make vertex and edge ids in digraph unique #9361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 11 additions & 20 deletions lib/stdlib/src/digraph.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@ new(Type) ->
V = ets:new(vertices, [set, Access]),
E = ets:new(edges, [set, Access]),
N = ets:new(neighbours, [bag, Access]),
ets:insert(N, [{'$vid', 0}, {'$eid', 0}]),
set_type(Ts, #digraph{vtab=V, etab=E, ntab=N});
error ->
erlang:error(badarg)
Expand Down Expand Up @@ -251,7 +250,7 @@ The created vertex is represented by term `['$v' | N]`, where `N` is an intege
G :: graph().

add_vertex(G) ->
do_add_vertex({new_vertex_id(G), []}, G).
do_add_vertex({new_vertex_id(), []}, G).

-doc(#{equiv => add_vertex(G, V, [])}).
-spec add_vertex(G, V) -> vertex() when
Expand Down Expand Up @@ -409,7 +408,7 @@ out_edges(G, V) ->
V2 :: vertex().

add_edge(G, V1, V2) ->
do_add_edge({new_edge_id(G), V1, V2, []}, G).
do_add_edge({new_edge_id(), V1, V2, []}, G).

-doc(#{equiv => add_edge/5}).
-doc """
Expand All @@ -426,7 +425,7 @@ See `t:add_edge_err_rsn/0` for details on possible errors.
Label :: label().

add_edge(G, V1, V2, D) ->
do_add_edge({new_edge_id(G), V1, V2, D}, G).
do_add_edge({new_edge_id(), V1, V2, D}, G).

-doc """
Creates (or modifies) an edge with the identifier
Expand Down Expand Up @@ -513,30 +512,22 @@ edge(G, E) ->
%%
%% Generate a "unique" edge identifier (relative to this graph)
%%
-spec new_edge_id(graph()) -> edge().
-spec new_edge_id() -> edge().

-dialyzer({no_improper_lists, new_edge_id/1}).
-dialyzer({no_improper_lists, new_edge_id/0}).

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].
new_edge_id() ->
['$e' | erlang:unique_integer()].
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@jhogberg jhogberg Jan 31, 2025 •
edited
Loading

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.

Copy link
Contributor

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.


%%
%% Generate a "unique" vertex identifier (relative to this graph)
%%
-spec new_vertex_id(graph()) -> vertex().
-spec new_vertex_id() -> vertex().

-dialyzer({no_improper_lists, new_vertex_id/1}).
-dialyzer({no_improper_lists, new_vertex_id/0}).

new_vertex_id(G) ->
NT = G#digraph.ntab,
[{'$vid', K}] = ets:lookup(NT, '$vid'),
true = ets:delete(NT, '$vid'),
true = ets:insert(NT, {'$vid', K+1}),
['$v' | K].
new_vertex_id() ->
['$v' | erlang:unique_integer()].

%%
%% Collect elements for a index in a tuple
Expand Down
2 changes: 2 additions & 0 deletions lib/stdlib/test/digraph_utils_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Contributor

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.

digraph:add_vertex(SG, b),
Copy link
Contributor

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 = digraph:delete(SG),

SG1 = digraph_utils:subgraph(G, [f, g, h],
Expand Down
Loading