Edgewall Software

Ticket #4988 (closed enhancement: worksforme)

Opened 20 months ago

Last modified 16 months ago

[PATCH] buffer SVN _get_history for better performance

Reported by: csapuntz@… Owned by: mgood
Priority: normal Milestone:
Component: version control Version: 0.10.3
Severity: normal Keywords: patch
Cc:

Description

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

Attachments

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

Change History

Changed 20 months ago by csapuntz@…

Diff to optimize _get_history in svn_fs.py

Changed 20 months ago by csapuntz@…

Second version of patch that tracks across path changes

follow-up: ↓ 2   Changed 20 months ago by mgood

  • keywords patch added
  • owner changed from cboos to mgood
  • summary changed from Fix svn_fs.py _get_history slow on larger repository to [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   Changed 20 months ago by anonymous

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: {{{ #!python 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.

  Changed 20 months ago by mgood

  • status changed from new to closed
  • resolution set to worksforme

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.

  Changed 20 months ago by cboos

(see r4711)

Add/Change #4988 ([PATCH] buffer SVN _get_history for better performance)

Author



Change Properties
<Author field>
Action
as closed
Next status will be 'reopened'
to The owner will change from mgood. Next status will be 'closed'
 
Note: See TracTickets for help on using tickets.