new editor interface?

Ben Collins and I were doing some imports, adds, commits, etc last night and
looking at SVN's memory usage. I found an unbounded growth on the add and
fixed that. 'svn add' now appears to have a constant working set size (I
added about 1600 files, the global pool seems to never have gone over about
50k).

Next up was 'commit'. The performance was just such a dog (with all the
logging and checking, me thinks) that I stopped it part way. However, there
is definitely a noticable growth over time. The main pool was at about 770k
and overall was up to about 12meg, when I stopped it.

Some initial investigation showed the standard culprit: file and dir batons
allocated from a global(ish) pool, rather than subpools.

To put this problem to a final rest, I've drafted a new editor interface. I
omitted the comments (for brevity) from the code pasted below, but the main
changes from the current interface are:

* rather than passing 'svn_stringbuf_t *name', we pass both a name and a
  relative path [to the editor root]. these are 'const char *'.

* when a baton is to be constructed, we pass a pool to use for that baton
  and any related data.

* close_directory() and close_file() are omitted. Instead, register a
  cleanup with the pool that was passed at baton construction time.

* property funcs take the "new style" types, rather than stringbuf

Note that this interface is a tiny bit tougher for the caller: both name and
path are needed, and pool handling must be performed. We could eliminate the
NAME parameters and just pass the path (strrchr(path, '/') is easily used to
find the name). The pool issue shouldn't be too hard: before calling
open/add, the caller allocates a pool and stores it away (usually, this is
just a local variable in a recursive situation), and destroys it rather than
calling close.

However, this interface also greatly eliminates the potential for poor pool
usage during editor walks (editors could still allocate from a global pool
via their edit baton, but hey). It makes it a lot easier for editors to Do
The Right Thing. It also has the relative path, which most editors put
together anyhow.

It is also possible to write a "mapper" which can map from the old call
interface to the new interface. Thus, a driver could continue to use the old
vtable. We then convert editors, one at a time, over to the new interface.
We then return an old-editor to the old-driver by wrapping this mapper
old-editor around the new-editor vtable. Once all editors for a given driver
are converted, then we can switch the driver to the new interface.

[ we could also write a reverse-mapper allowing drivers to switch, yet still
  interface with old editors ]

This gives us a phased approach, where we get the benefits of the new
interface for each editor we convert. We don't have to revamp the world at
once.

Thoughts?

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
typedef struct
{
  svn_error_t *(*set_target_revision) (void *edit_baton,
                                       svn_revnum_t target_revision);
					 
  svn_error_t *(*open_root) (void *edit_baton,
                             svn_revnum_t base_revision,
                             apr_pool_t *dir_pool,
			     void **root_baton);
							  
  svn_error_t *(*delete_entry) (const char *name,
                                const char *path,
				svn_revnum_t revision,
				void *parent_baton);
  svn_error_t *(*add_directory) (const char *name,
                                 const char *path,
				 void *parent_baton,
				 const char *copyfrom_path,
				 svn_revnum_t copyfrom_revision,
                                 apr_pool_t *dir_pool,
				 void **child_baton);
  svn_error_t *(*open_directory) (const char *name,
                                  const char *path,
				  void *parent_baton,
				  svn_revnum_t base_revision,
				  apr_pool_t *dir_pool,
				  void **child_baton);
  svn_error_t *(*change_dir_prop) (void *dir_baton,
                                   const char *name,
				   const svn_string_t *value);
  svn_error_t *(*add_file) (const char *name,
                            const char *path,
			    void *parent_baton,
			    const char *copy_path,
			    svn_revnum_t copy_revision,
			    apr_pool_t *file_pool,
                            void **file_baton);
  svn_error_t *(*open_file) (const char *name,
                             const char *path,
			     void *parent_baton,
			     svn_revnum_t base_revision,
			     apr_pool_t *file_pool,
			     void **file_baton);
  svn_error_t *(*apply_textdelta) (void *file_baton, 
                                   svn_txdelta_window_handler_t *handler,
                                   void **handler_baton);
  svn_error_t *(*change_file_prop) (void *file_baton,
                                    const char *name,
                                    const svn_string_t *value);
				    svn_error_t *(*close_edit) (void *edit_baton);
				    svn_error_t *(*abort_edit) (void *edit_baton);
} svn_delta_editor_t;  
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org