[SERVER-27842] Replace BSONElement.fieldName() calls with BSONElement.fieldNameStringData() in dotted_path_support.cpp Created: 27/Jan/17  Updated: 05/Apr/17  Resolved: 10/Mar/17

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 3.5.5

Type: Improvement Priority: Major - P3
Reporter: James Wahlin Assignee: Nicholas Zolnierz
Resolution: Done Votes: 0
Labels: neweng
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Backports
Related
related to SERVER-27846 Improve performance of the update pat... Backlog
Backwards Compatibility: Fully Compatible
Backport Requested:
v3.4
Sprint: Query 2017-03-27
Participants:

 Description   

Currently we extract field name via fieldName() call, which returns a c-string. With this we lose size information for the field name and have to calculate again when passing to methods that take name as StringData. Calling fieldNameStringData() allows us to avoid duplicated efforts.

As part of this work, we should also consider removing the BSONElement::fieldName() method entirely.



 Comments   
Comment by Githook User [ 10/Mar/17 ]

Author:

{u'name': u'nzolnierzmdb', u'email': u'nicholas.zolnierz@10gen.com'}

Message: SERVER-27842 Replace BSONElement.fieldName() calls with BSONElement.fieldNameStringData() in dotted_path_support.cpp
Branch: master
https://github.com/mongodb/mongo/commit/d2c44fd6ccdeab0590f8fe2413c84c2467f2b60e

Comment by James Wahlin [ 09/Mar/17 ]

In regards to investigating removal of BSONElement::fieldName(), a review of callers shows that use of fieldNameStringData() over fieldName() would be detrimental in some cases. This is true when we have not evaluated a BSONElement field name size and perform an operation that does not require the field name size. For example, in the following we avoid a call to strlen() on the field name by using fieldName() over fieldNameStringData():

// e is of type BSONElement, where field name size has not yet been calculated
 
// A call to fieldName() returns a char* and we compare the first character to '$'
str::startsWith(e.fieldName(), '$');
 
// A call to fieldNameStringData() will create a StringData object and calculate the field name size in the constructor. We then compare the first character of the underlying char* to '$'
str::startsWith(e.fieldNameStringData(), '$')

Given this, I don't think we should remove the fieldName() method from BSONElement, but should be careful in our usage.

Comment by James Wahlin [ 27/Jan/17 ]

The following is patch for this change, based off of commit c91b93d2786342505fd9e151c8aa6b68ee03a1fb:

diff --git a/src/mongo/db/bson/dotted_path_support.cpp b/src/mongo/db/bson/dotted_path_support.cpp
index 1a9fda8..055a240 100644
--- a/src/mongo/db/bson/dotted_path_support.cpp
+++ b/src/mongo/db/bson/dotted_path_support.cpp
@@ -194,11 +194,13 @@ BSONObj extractElementsBasedOnTemplate(const BSONObj& obj,
         BSONElement e = i.next();
         if (e.eoo())
             break;
-        BSONElement x = extractElementAtPath(obj, e.fieldName());
+
+        const auto name = e.fieldNameStringData();
+        BSONElement x = extractElementAtPath(obj, name);
         if (!x.eoo())
-            b.appendAs(x, e.fieldName());
+            b.appendAs(x, name);
         else if (useNullIfMissing)
-            b.appendNull(e.fieldName());
+            b.appendNull(name);
     }
     return b.obj();
 }
@@ -220,12 +222,13 @@ int compareObjectsAccordingToSort(const BSONObj& firstObj,
         if (f.eoo())
             return 0;
 
-        BSONElement l = assumeDottedPaths ? extractElementAtPath(firstObj, f.fieldName())
-                                          : firstObj.getField(f.fieldName());
+        const auto name = e.fieldNameStringData();
+        BSONElement l = assumeDottedPaths ? extractElementAtPath(firstObj, name)
+                                          : firstObj.getField(name);
         if (l.eoo())
             l = kNullElt;
-        BSONElement r = assumeDottedPaths ? extractElementAtPath(secondObj, f.fieldName())
-                                          : secondObj.getField(f.fieldName());
+        BSONElement r = assumeDottedPaths ? extractElementAtPath(secondObj, name)
+                                          : secondObj.getField(name);
         if (r.eoo())
             r = kNullElt;

Generated at Thu Feb 08 04:16:24 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.