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.

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

Waste of time in iterator_edges 3 #16220

Closed
videlec opened this issue Apr 23, 2014 · 16 comments
Closed

Waste of time in iterator_edges 3 #16220

videlec opened this issue Apr 23, 2014 · 16 comments

Comments

@videlec
Copy link
Contributor

videlec commented Apr 23, 2014

Further optimizations for graph iterations following #16005.

The sparse graph backend (i.e. sage.graphs.base.sparse_graph.SparseGraph and sage.graphs.base.sparse_graph.SparseGraphBackend) lacks optimization especially for edge iteration. We need to see what is the best we can do and work on it!

CC: @nathanncohen

Component: graph theory

Keywords: datastructure

Reviewer: David Coudert

Issue created by migration from https://trac.sagemath.org/ticket/16220

@videlec videlec added this to the sage-6.2 milestone Apr 23, 2014
@videlec videlec self-assigned this Apr 23, 2014
@videlec

This comment has been minimized.

@videlec
Copy link
Contributor Author

videlec commented Apr 23, 2014

Branch: public/16220

@videlec
Copy link
Contributor Author

videlec commented Apr 23, 2014

Commit: 98286f4

@videlec
Copy link
Contributor Author

videlec commented Apr 23, 2014

Changed keywords from none to datastructure

@videlec
Copy link
Contributor Author

videlec commented Apr 23, 2014

New commits:

56ec117trac 16005: Waste of time in iterator_edges 2
567600btrac #16005: merge into 6.2.beta8
9623fd7Merge branch 'u/ncohen/16005' of trac.sagemath.org:sage into 16005-waste_of_times_graph2
98286f4optimization of edge_iteration in SparseGraphBackend + documentation

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Apr 23, 2014

comment:2

Isn't there a problem if you want to iterate over the edges but stop midway ? Isn't it the case that the array is never deallocated ?

Nathann

@videlec
Copy link
Contributor Author

videlec commented Apr 23, 2014

comment:3

Replying to @nathanncohen:

Isn't there a problem if you want to iterate over the edges but stop midway ? Isn't it the case that the array is never deallocated ?

You are right, with a code like

sage: it = build_me_an_edge_iterator()
sage: it.next()
sage: del it

we keep an array never deallocated. But what happens for alloc/dealloc inside cdef functions?

A solution would be to implement an iterator in Cython with direct access to the SparseGraph (doing all proper deallocation in __dealloc__). Is it worth it?

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Apr 23, 2014

comment:4

we keep an array never deallocated. But what happens for alloc/dealloc inside cdef functions?

I would say that it never happens, and that the only way is to create a class with a constructor/destructor. Which is a lot of code.

A solution would be to implement an iterator in Cython with direct access to the SparseGraph (doing all proper deallocation in __dealloc__). Is it worth it?

Well, the question is whether an iterator is cheaper than a malloc.

Nathann

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@kliem
Copy link
Contributor

kliem commented Sep 29, 2020

comment:7

#30665 does some improvements to the edge iterator. It is surely not optimized out. Not sure, if we want to close this ticket here. Or keep it for further improvements.

@dcoudert
Copy link
Contributor

dcoudert commented Oct 2, 2020

comment:8

We can certainly close it.

@kliem
Copy link
Contributor

kliem commented Oct 2, 2020

comment:9

With #30665, I think this can be closed. The branch certainly doesn't work anymore after redesign.

@kliem kliem removed this from the sage-6.4 milestone Oct 2, 2020
@dcoudert
Copy link
Contributor

dcoudert commented Oct 3, 2020

Reviewer: David Coudert

@dcoudert
Copy link
Contributor

dcoudert commented Oct 3, 2020

comment:10

Agreed.

@slel
Copy link
Member

slel commented Oct 11, 2020

Changed branch from public/16220 to none

@slel
Copy link
Member

slel commented Oct 11, 2020

Changed author from Vincent Delecroix to none

@slel
Copy link
Member

slel commented Oct 11, 2020

Changed commit from 98286f4 to none

@slel slel removed the p: major / 3 label Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants