Edgewall Software
Modify

Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#4988 closed enhancement (worksforme)

[PATCH] buffer SVN _get_history for better performance

Reported by: csapuntz@… Owned by: Matthew Good
Priority: normal Milestone:
Component: version control Version: 0.10.3
Severity: normal Keywords: patch
Cc: Branch:
Release Notes:
API Changes:

Description

This leads to slow bitten build status pages and maybe other places in trac.

Attachments (2)

svn_fs.py.diff (2.7 KB ) - added by csapuntz@… 13 years ago.
Diff to optimize _get_history in svn_fs.py
svn_fs.py.2.diff (2.8 KB ) - added by csapuntz@… 13 years ago.
Second version of patch that tracks across path changes

Download all attachments as: .zip

Change History (6)

by csapuntz@…, 13 years ago

Attachment: svn_fs.py.diff added

Diff to optimize _get_history in svn_fs.py

by csapuntz@…, 13 years ago

Attachment: svn_fs.py.2.diff added

Second version of patch that tracks across path changes

comment:1 by Matthew Good, 13 years ago

Keywords: patch added
Owner: changed from Christian Boos to Matthew Good
Summary: Fix svn_fs.py _get_history slow on larger repository[PATCH] buffer SVN _get_history for better performance

Yeah, this buffered approach looks good, but I've got a couple of questions/comments.

  • What is the purpose of the try/except block? I'd like to have a comment in the code explaining why the exception is being discarded, if that really is necessary.
  • Instead of the changes made between the two versions of the patch it looks like you could just add this line after the "for item in history" loop:
    svn_path = _to_svn(item[0])
    
  • The "if hasattr(…)" and nested function definitions should be pulled out of the loop since it's redundant to recheck and redefine these each iteration.

I can make those changes to the patch if you like, but I'd mainly like to know about the exceptions getting discarded before committing this.

in reply to:  1 comment:2 by anonymous, 13 years ago

It looks like this problem may have already been fixed in 0.10.4dev. The _get_history is gone.

Replying to mgood:

  • What is the purpose of the try/except block? I'd like to have a comment in the code explaining why the exception is being discarded, if that really is necessary.

If there are no revisions in the range, svn_repos_history2 throws an exception.

  • Instead of the changes made between the two versions of the patch it looks like you could just add this line after the "for item in history" loop:
    svn_path = _to_svn(item[0])
    

Sounds reasonable. Need to check that item is not None too?

  • The "if hasattr(…)" and nested function definitions should be pulled out of the loop since it's redundant to recheck and redefine these each iteration.

My python isn't good so I don't feel confident attempting that. Also, I would guess the cost of that check and function definitions is dwarfed by the cost of calling svn_repos_history2.

I can make those changes to the patch if you like, but I'd mainly like to know about the exceptions getting discarded before committing this.

Also, I think subversion 1.2 and before throw SystemError so the except should also filter that.

comment:3 by Matthew Good, 13 years ago

Resolution: worksforme
Status: newclosed

Yes, it appears that some with recent changes the code now iterates over the history instead of pulling all revisions for a path into memory which should solve the same issue this patch was meant to solve.

comment:4 by Christian Boos, 13 years ago

(see r4711)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Matthew Good.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Matthew Good to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.