This change, and to a lesser extent the one you did for Locks, could
potentially be a performance issue for Subclipse. Status API is
pretty important and the info is cached. This change basically means
we have to do a separate SVN info API call for every status result. I
have to think that is going to have measurable impact on performance.
I have been working through the data structures to see if I could
remove this extra information from the object and then only call the
Info API when it is needed. However, this looks like it is going to
be a lot more difficult than I hoped. Perhaps even impossible.
For example, I have to know if items have conflicts because that
drives the decorators. That is not too bad, because the extra info
calls only happens when we know the item has a conflict.
The copiedFrom info is more difficult. We use and need that info in
some more subtle ways where it gets harder to defer the cost.
It looks like you are removing this info to defer the cost of some
additional API calls. I am generally happy to see you looking into
those kind of optimizations but in this case I am not sure if the
result is worth it. It looks like you are removing some fairly simple
C++ code that is probably relatively lightweight (could be wrong) and
basically making callers perform a very heavy alternative to still get
that data.
Mark
On Fri, May 6, 2011 at 2:33 PM, <hwright_at_apache.org> wrote:
> Author: hwright
> Date: Fri May 6 18:33:23 2011
> New Revision: 1100316
>
> URL: http://svn.apache.org/viewvc?rev=1100316&view=rev
> Log:
> JavaHL: Don't send copy-from info as part of status. It isn't provided
> by the C struct, and interested callers can fetch it a second way.
>
> (We still do inform if a node has been copied or not.)
>
> [ in subversion/bindings/javahl/ ]
> * native/CreateJ.cpp
> (Status): Don't fetch or send copy-from information.
>
> * src/org/apache/subversion/javahl/types/Status.java
> (urlCopiedFrom, revisionCopiedFrom): Remove, with getters.
> (Status): Adjust args, don't save copy-from info.
>
> * src/org/tigris/subversion/javahl/Status.java
> (populateConflicts): Rename to...
> (populateFromInfo): ...this, and also set the copy-from information.
> (Status): Update constructor args, and call the renamed function.
>
> Modified:
> subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp
> subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/javahl/types/Status.java
> subversion/trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/Status.java
>
> Modified: subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp?rev=1100316&r1=1100315&r2=1100316&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp (original)
> +++ subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp Fri May 6 18:33:23 2011
> @@ -501,8 +501,7 @@ CreateJ::Status(svn_wc_context_t *wc_ctx
> "L"JAVA_PACKAGE"/types/Status$Kind;"
> "L"JAVA_PACKAGE"/types/Status$Kind;"
> "L"JAVA_PACKAGE"/types/Status$Kind;"
> - "ZZZLjava/lang/String;JZZ"
> - "L"JAVA_PACKAGE"/types/Lock;"
> + "ZZZZZL"JAVA_PACKAGE"/types/Lock;"
> "L"JAVA_PACKAGE"/types/Lock;"
> "JJL"JAVA_PACKAGE"/types/NodeKind;"
> "Ljava/lang/String;Ljava/lang/String;)V");
> @@ -522,9 +521,6 @@ CreateJ::Status(svn_wc_context_t *wc_ctx
> org_apache_subversion_javahl_types_Revision_SVN_INVALID_REVNUM;
> jlong jLastChangedDate = 0;
> jstring jLastCommitAuthor = NULL;
> - jstring jURLCopiedFrom = NULL;
> - jlong jRevisionCopiedFrom =
> - org_apache_subversion_javahl_types_Revision_SVN_INVALID_REVNUM;
> jobject jLocalLock = NULL;
> jstring jChangelist = NULL;
>
> @@ -593,35 +589,11 @@ CreateJ::Status(svn_wc_context_t *wc_ctx
> POP_AND_RETURN_NULL;
> }
>
> - if (status->versioned && status->conflicted)
> - {
> - const char *copyfrom_url;
> - svn_revnum_t copyfrom_rev;
> - svn_boolean_t is_copy_target;
> -
> - SVN_JNI_ERR(svn_wc__node_get_copyfrom_info(NULL, NULL,
> - ©from_url,
> - ©from_rev,
> - &is_copy_target,
> - wc_ctx,
> - status->local_abspath,
> - pool, pool), NULL);
> -
> - jURLCopiedFrom = JNIUtil::makeJString(is_copy_target ? copyfrom_url
> - : NULL);
> - if (JNIUtil::isJavaExceptionThrown())
> - POP_AND_RETURN_NULL;
> -
> - jRevisionCopiedFrom = is_copy_target ? copyfrom_rev
> - : SVN_INVALID_REVNUM;
> - }
> -
> jobject ret = env->NewObject(clazz, mid, jPath, jUrl, jNodeKind, jRevision,
> jLastChangedRevision, jLastChangedDate,
> jLastCommitAuthor, jTextType, jPropType,
> jRepositoryTextType, jRepositoryPropType,
> jIsLocked, jIsCopied, jIsConflicted,
> - jURLCopiedFrom, jRevisionCopiedFrom,
> jIsSwitched, jIsFileExternal, jLocalLock,
> jReposLock,
> jOODLastCmtRevision, jOODLastCmtDate,
>
> Modified: subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/javahl/types/Status.java
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/javahl/types/Status.java?rev=1100316&r1=1100315&r2=1100316&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/javahl/types/Status.java (original)
> +++ subversion/trunk/subversion/bindings/javahl/src/org/apache/subversion/javahl/types/Status.java Fri May 6 18:33:23 2011
> @@ -127,16 +127,6 @@ public class Status implements java.io.S
> private Kind repositoryPropStatus;
>
> /**
> - * if copied, the url of the copy source
> - */
> - private String urlCopiedFrom;
> -
> - /**
> - * if copied, the revision number of the copy source
> - */
> - private long revisionCopiedFrom;
> -
> - /**
> * the current lock
> */
> private Lock localLock;
> @@ -200,9 +190,6 @@ public class Status implements java.io.S
> * repository version
> * @param conflictWorking in case of conflict, the file name of the
> * former working copy version
> - * @param urlCopiedFrom if copied, the url of the copy source
> - * @param revisionCopiedFrom if copied, the revision number of the copy
> - * source
> * @param switched flag if the node has been switched in the
> * path
> * @param fileExternal flag if the node is a file external
> @@ -222,7 +209,6 @@ public class Status implements java.io.S
> String lastCommitAuthor, Kind textStatus, Kind propStatus,
> Kind repositoryTextStatus, Kind repositoryPropStatus,
> boolean locked, boolean copied, boolean isConflicted,
> - String urlCopiedFrom, long revisionCopiedFrom,
> boolean switched, boolean fileExternal, Lock localLock,
> Lock reposLock, long reposLastCmtRevision,
> long reposLastCmtDate, NodeKind reposKind,
> @@ -242,8 +228,6 @@ public class Status implements java.io.S
> this.isConflicted = isConflicted;
> this.repositoryTextStatus = repositoryTextStatus;
> this.repositoryPropStatus = repositoryPropStatus;
> - this.urlCopiedFrom = urlCopiedFrom;
> - this.revisionCopiedFrom = revisionCopiedFrom;
> this.switched = switched;
> this.fileExternal = fileExternal;
> this.localLock = localLock;
> @@ -428,33 +412,6 @@ public class Status implements java.io.S
> }
>
> /**
> - * Returns if copied the copy source url or null
> - * @return the source url
> - */
> - public String getUrlCopiedFrom()
> - {
> - return urlCopiedFrom;
> - }
> -
> - /**
> - * Returns if copied the source revision as a Revision object
> - * @return the source revision
> - */
> - public Revision.Number getRevisionCopiedFrom()
> - {
> - return Revision.createNumber(revisionCopiedFrom);
> - }
> -
> - /**
> - * Returns if copied the source revision as s long integer
> - * @return the source revision
> - */
> - public long getRevisionCopiedFromNumber()
> - {
> - return revisionCopiedFrom;
> - }
> -
> - /**
> * Returns if the repository url has been switched
> * @return is the item has been switched
> */
>
> Modified: subversion/trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/Status.java
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/Status.java?rev=1100316&r1=1100315&r2=1100316&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/Status.java (original)
> +++ subversion/trunk/subversion/bindings/javahl/src/org/tigris/subversion/javahl/Status.java Fri May 6 18:33:23 2011
> @@ -331,8 +331,8 @@ public class Status implements java.io.S
> }
>
> private void
> - populateConflicts(org.apache.subversion.javahl.SVNClient aClient,
> - String path)
> + populateFromInfo(org.apache.subversion.javahl.SVNClient aClient,
> + String path)
> throws org.apache.subversion.javahl.ClientException
> {
> class MyInfoCallback
> @@ -357,31 +357,35 @@ public class Status implements java.io.S
> org.apache.subversion.javahl.types.Depth.empty, null,
> callback);
>
> - if (callback.getInfo() == null
> - || callback.getInfo().getConflicts() == null)
> + org.apache.subversion.javahl.types.Info aInfo = callback.getInfo();
> + if (aInfo == null)
> return;
>
> - for (org.apache.subversion.javahl.ConflictDescriptor conflict
> - : callback.getInfo().getConflicts())
> - {
> - switch (conflict.getKind())
> - {
> - case tree:
> - this.treeConflicted = true;
> - this.conflictDescriptor = new ConflictDescriptor(conflict);
> - break;
> -
> - case text:
> - this.conflictOld = conflict.getBasePath();
> - this.conflictWorking = conflict.getMergedPath();
> - this.conflictNew = conflict.getMyPath();
> - break;
> -
> - case property:
> - // Ignore
> - break;
> - }
> - }
> + if (aInfo.getConflicts() != null)
> + for (org.apache.subversion.javahl.ConflictDescriptor conflict
> + : aInfo.getConflicts())
> + {
> + switch (conflict.getKind())
> + {
> + case tree:
> + this.treeConflicted = true;
> + this.conflictDescriptor = new ConflictDescriptor(conflict);
> + break;
> +
> + case text:
> + this.conflictOld = conflict.getBasePath();
> + this.conflictWorking = conflict.getMergedPath();
> + this.conflictNew = conflict.getMyPath();
> + break;
> +
> + case property:
> + // Ignore
> + break;
> + }
> + }
> +
> + this.urlCopiedFrom = aInfo.getCopyFromUrl();
> + this.revisionCopiedFrom = aInfo.getCopyFromRev();
> }
>
> void
> @@ -412,8 +416,8 @@ public class Status implements java.io.S
> fromAStatusKind(aStatus.getRepositoryTextStatus()),
> fromAStatusKind(aStatus.getRepositoryPropStatus()),
> aStatus.isLocked(), aStatus.isCopied(), false,
> - null, null, null, null, aStatus.getUrlCopiedFrom(),
> - aStatus.getRevisionCopiedFromNumber(), aStatus.isSwitched(),
> + null, null, null, null, null, Revision.SVN_INVALID_REVNUM,
> + aStatus.isSwitched(),
> aStatus.isFileExternal(), null, null, null, 0,
> aStatus.getReposLock() == null ? null
> : new Lock(aStatus.getReposLock()),
> @@ -423,8 +427,7 @@ public class Status implements java.io.S
> aStatus.getReposLastCmtAuthor(), aStatus.getChangelist());
>
> try {
> - if (aStatus.isConflicted())
> - populateConflicts(aClient, aStatus.getPath());
> + populateFromInfo(aClient, aStatus.getPath());
> if (aStatus.getLocalLock() != null)
> populateLocalLock(aStatus.getLocalLock());
> } catch (org.apache.subversion.javahl.ClientException ex) {
>
>
>
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Received on 2011-05-06 23:00:32 CEST