Skip to content

Commit e1abb1d

Browse files
JeremyCaneyJeremyCaney
committed
Merge branch 'improvement/AttributeDictionary-performance' into develop
Established a threshold of attributes (of five) before invoking the `AttributeDictionary` constructor during the `TopicMappingService`, if available (per Feature #99). There is overhead to establishing a new `AttributeDictionary` via, especially, `AttributeCollection.AsAttributeDictionary(true)`. This pays off after just one or two mapped attributes. However, most topics have multiple attributes which will not be mapped (e.g., `LastModifiedBy`) or are excluded (e.g., `LastModified`, `Title`), and thus shouldn't be counted. As a result, setting the threshold at five generally achieves a useful heuristic around improving performance without worrying about the overhead. As part of this, I introduced "unit tests" which can be configured to aid in identifying where this threshold exists, and whether or not updates to the `TopicMappingService` are performant or not.
2 parents afd6c87 + 2e157e5 commit e1abb1d

3 files changed

Lines changed: 254 additions & 6 deletions

File tree

‎OnTopic.Tests/TopicMappingServiceTest.cs‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,140 @@ public TopicMappingServiceTest(TopicInfrastructureFixture<StubTopicRepository> f
6464

6565
}
6666

67+
/*==========================================================================================================================
68+
| TEST: MAP: LOAD TESTING: EVALUATE THRESHOLD
69+
\-------------------------------------------------------------------------------------------------------------------------*/
70+
/// <summary>
71+
/// Establishes a <see cref="TopicMappingService"/> and tests creating and mapping <see cref="Topic"/> instances in bulk,
72+
/// as a quick-and-easy way of assessing performance.
73+
/// </summary>
74+
/// <remarks>
75+
/// <para>
76+
/// The <see cref="TopicMappingService"/> includes functionality to map properties to attributes via a constructor that
77+
/// accepts a <see cref="AttributeDictionary"/>. This introduces some overhead which is not cost effective if there are
78+
/// not any attributes that map to properties. For larger numbers of mapped attributes, however, the <see cref="
79+
/// AttributeDictionary"/> can reduce the mapping time considerably, while also giving more control over the model
80+
/// construction to the model developer. This test is intended to help identify and optimize that threshold based on
81+
/// improvements to the underlying <see cref="AttributeDictionary"/>, <see cref="TopicMappingService.MapAsync(Topic?,
82+
/// AssociationTypes)"/>, and <see cref="AttributeCollection.AsAttributeDictionary(bool)"/> convenience method.
83+
/// </para>
84+
/// <para>
85+
/// This is only intended to be enabled when needed for specialized performance testing.
86+
/// </para>
87+
/// </remarks>
88+
[Fact]
89+
public async Task Map_Bulk_EvaluateThreshold() {
90+
91+
/*------------------------------------------------------------------------------------------------------------------------
92+
| Establish variables
93+
\-----------------------------------------------------------------------------------------------------------------------*/
94+
var runs = 0; // The number of mapping operations to perform
95+
var propertyCount = 10; // The number of property values to set on the LoadTestingModel
96+
97+
/*------------------------------------------------------------------------------------------------------------------------
98+
| Establish data model
99+
\-----------------------------------------------------------------------------------------------------------------------*/
100+
var topic = new Topic("Test", "ContentList", null);
101+
102+
for (var i = 0; i <= propertyCount; i++) {
103+
topic.Attributes.SetInteger("Property"+i, i);
104+
}
105+
106+
/*------------------------------------------------------------------------------------------------------------------------
107+
| Run load testing
108+
\-----------------------------------------------------------------------------------------------------------------------*/
109+
for (var i = 0; i < runs; i++) {
110+
await _mappingService.MapAsync<LoadTestingViewModel>(topic).ConfigureAwait(false);
111+
}
112+
113+
/*------------------------------------------------------------------------------------------------------------------------
114+
| Always assume the test passed
115+
\-----------------------------------------------------------------------------------------------------------------------*/
116+
Assert.True(true);
117+
118+
}
119+
120+
/*==========================================================================================================================
121+
| TEST: MAP: LOAD TESTING: EVALUATE TIME
122+
\-------------------------------------------------------------------------------------------------------------------------*/
123+
/// <summary>
124+
/// Establishes a <see cref="TopicMappingService"/> and tests creating and mapping <see cref="Topic"/> instances in bulk,
125+
/// as a quick-and-easy way of assessing performance.
126+
/// </summary>
127+
[Fact]
128+
public async Task Map_LoadTesting_EvaluateTime() {
129+
130+
/*------------------------------------------------------------------------------------------------------------------------
131+
| Establish variables
132+
\-----------------------------------------------------------------------------------------------------------------------*/
133+
var runs = 0; // The number of mapping operations to perform
134+
var useFullAttributeSet = false; // Include a larget set of attribute values
135+
var includeNestedTopics = false; // Only include a minimal set of properties
136+
137+
/*------------------------------------------------------------------------------------------------------------------------
138+
| Establish object model
139+
\-----------------------------------------------------------------------------------------------------------------------*/
140+
var currentId = 1;
141+
var baseTopic = new Topic("Base", "Page", null, currentId++);
142+
var grandparent = new Topic("Grandparent", "Page", null, currentId++);
143+
var parent = new Topic("Parent", "Page", grandparent, currentId++);
144+
var topic = new Topic("Test", "ContentList", parent, currentId++);
145+
var contentItems = new Topic("ContentItems", "List", topic, currentId++);
146+
147+
if (includeNestedTopics) {
148+
_ = new Topic("Item1", "ContentItem", contentItems, currentId++);
149+
_ = new Topic("Item2", "ContentItem", contentItems, currentId++);
150+
_ = new Topic("Item3", "ContentItem", contentItems, currentId++);
151+
_ = new Topic("Item4", "ContentItem", contentItems, currentId++);
152+
_ = new Topic("Item5", "ContentItem", contentItems, currentId++);
153+
_ = new Topic("Item6", "ContentItem", contentItems, currentId++);
154+
}
155+
156+
grandparent.BaseTopic = baseTopic;
157+
158+
/*------------------------------------------------------------------------------------------------------------------------
159+
| Populate attributes
160+
\-----------------------------------------------------------------------------------------------------------------------*/
161+
foreach (var contentItem in contentItems.Children) {
162+
contentItem.Attributes.SetDateTime("LastModified", DateTime.Now);
163+
contentItem.Attributes.SetValue("Description", "Value1");
164+
if (useFullAttributeSet) {
165+
contentItem.Attributes.SetValue("LearnMoreUrl", "/Topic/23/");
166+
contentItem.Attributes.SetValue("ThumbnailImage", "/Image.jpg");
167+
contentItem.Attributes.SetValue("Description", "Value1");
168+
contentItem.Attributes.SetValue("Category", "Gumby");
169+
}
170+
}
171+
172+
topic.Attributes.SetValue("Title", "Friendly Title");
173+
topic.Attributes.SetValue("IsHidden", "0");
174+
if (useFullAttributeSet) {
175+
topic.Attributes.SetValue("View", "Test");
176+
topic.Attributes.SetValue("ShortTitle", "Short Title");
177+
topic.Attributes.SetValue("Subtitle", "Subtitle");
178+
topic.Attributes.SetValue("MetaTitle", "Meta Title");
179+
topic.Attributes.SetValue("MetaDescription", "Meta Description");
180+
topic.Attributes.SetValue("MetaKeywords", "Load;Test;Keywords");
181+
topic.Attributes.SetValue("NoIndex", "0");
182+
topic.Attributes.SetValue("Body", "Body of test topic");
183+
}
184+
185+
baseTopic.Attributes.SetValue("Title", "Inherited Title");
186+
187+
/*------------------------------------------------------------------------------------------------------------------------
188+
| Run load testing
189+
\-----------------------------------------------------------------------------------------------------------------------*/
190+
for (var i = 0; i <= runs; i++) {
191+
await _mappingService.MapAsync(topic).ConfigureAwait(false);
192+
}
193+
194+
/*------------------------------------------------------------------------------------------------------------------------
195+
| Always assume the test passed
196+
\-----------------------------------------------------------------------------------------------------------------------*/
197+
Assert.True(true);
198+
199+
}
200+
67201
/*==========================================================================================================================
68202
| TEST: MAP: GENERIC: RETURNS NEW MODEL
69203
\-------------------------------------------------------------------------------------------------------------------------*/
@@ -1444,6 +1578,7 @@ public async Task Map_FlattenAttribute_ExcludeTopics() {
14441578
Assert.Single(target?.Children);
14451579

14461580
}
1581+
14471582
/*==========================================================================================================================
14481583
| TEST: MAP: CACHED TOPIC: RETURNS CACHED MODEL
14491584
\-------------------------------------------------------------------------------------------------------------------------*/

‎OnTopic.Tests/ViewModels/LoadTestingViewModel.cs‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
/*==============================================================================================================================
2+
| Author Ignia, LLC
3+
| Client Ignia, LLC
4+
| Project Topics Library
5+
\=============================================================================================================================*/
6+
using OnTopic.Attributes;
7+
using OnTopic.Mapping;
8+
9+
namespace OnTopic.Tests.ViewModels {
10+
11+
/*============================================================================================================================
12+
| VIEW MODEL: LOAD TESTING
13+
\---------------------------------------------------------------------------------------------------------------------------*/
14+
/// <summary>
15+
/// Provides a simple view model with a series of properties that can be used for load testing the <see cref="
16+
/// TopicMappingService"/>.
17+
/// </summary>
18+
/// <remarks>
19+
/// This is a sample class intended for test purposes only; it is not designed for use in a production environment.
20+
/// </remarks>
21+
public class LoadTestingViewModel: KeyOnlyTopicViewModel {
22+
23+
/*==========================================================================================================================
24+
| CONSTRUCTOR
25+
\-------------------------------------------------------------------------------------------------------------------------*/
26+
/// <summary>
27+
/// Initializes a new <see cref="LoadTestingViewModel"/> with an <paramref name="attributes"/> dictionary.
28+
/// </summary>
29+
/// <param name="attributes">An <see cref="AttributeDictionary"/> of attribute values.</param>
30+
public LoadTestingViewModel(AttributeDictionary attributes) {
31+
Contract.Requires(attributes);
32+
Property0 = attributes.GetInteger("Property0");
33+
Property1 = attributes.GetInteger("Property1");
34+
Property2 = attributes.GetInteger("Property2");
35+
Property3 = attributes.GetInteger("Property3");
36+
Property4 = attributes.GetInteger("Property4");
37+
Property5 = attributes.GetInteger("Property5");
38+
Property6 = attributes.GetInteger("Property6");
39+
Property7 = attributes.GetInteger("Property7");
40+
Property8 = attributes.GetInteger("Property8");
41+
Property9 = attributes.GetInteger("Property9");
42+
Property10 = attributes.GetInteger("Property10");
43+
Property11 = attributes.GetInteger("Property11");
44+
Property12 = attributes.GetInteger("Property12");
45+
Property13 = attributes.GetInteger("Property13");
46+
Property14 = attributes.GetInteger("Property14");
47+
Property15 = attributes.GetInteger("Property15");
48+
Property16 = attributes.GetInteger("Property16");
49+
Property17 = attributes.GetInteger("Property17");
50+
Property18 = attributes.GetInteger("Property18");
51+
Property19 = attributes.GetInteger("Property19");
52+
Property20 = attributes.GetInteger("Property20");
53+
}
54+
55+
/// <summary>
56+
/// Initializes a new <see cref="PageTopicViewModel"/> with no parameters.
57+
/// </summary>
58+
public LoadTestingViewModel() { }
59+
60+
/*==========================================================================================================================
61+
| PROPERTIES
62+
\-------------------------------------------------------------------------------------------------------------------------*/
63+
public int? Property0 { get; init; }
64+
public int? Property1 { get; init; }
65+
public int? Property2 { get; init; }
66+
public int? Property3 { get; init; }
67+
public int? Property4 { get; init; }
68+
public int? Property5 { get; init; }
69+
public int? Property6 { get; init; }
70+
public int? Property7 { get; init; }
71+
public int? Property8 { get; init; }
72+
public int? Property9 { get; init; }
73+
public int? Property10 { get; init; }
74+
public int? Property11 { get; init; }
75+
public int? Property12 { get; init; }
76+
public int? Property13 { get; init; }
77+
public int? Property14 { get; init; }
78+
public int? Property15 { get; init; }
79+
public int? Property16 { get; init; }
80+
public int? Property17 { get; init; }
81+
public int? Property18 { get; init; }
82+
public int? Property19 { get; init; }
83+
public int? Property20 { get; init; }
84+
85+
} //Class
86+
} //Namespace

‎OnTopic/Mapping/TopicMappingService.cs‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -170,9 +170,11 @@ public TopicMappingService(ITopicRepository topicRepository, ITypeLookupService
170170
| Identify parameters
171171
\-----------------------------------------------------------------------------------------------------------------------*/
172172
var typeAccessor = TypeAccessorCache.GetTypeAccessor(type);
173+
var properties = typeAccessor.GetMembers(MemberTypes.Property);
173174
var parameters = typeAccessor.ConstructorParameters;
174175
var arguments = new object?[parameters.Count];
175176
var attributeArguments = (IDictionary<string, string?>)new Dictionary<string, string?>();
177+
var parameterQueue = new Dictionary<int, Task<object?>>();
176178

177179
/*------------------------------------------------------------------------------------------------------------------------
178180
| Pre-cache entry
@@ -185,15 +187,40 @@ public TopicMappingService(ITopicRepository topicRepository, ITypeLookupService
185187
cache.Preregister(topic.Id, type);
186188

187189
/*------------------------------------------------------------------------------------------------------------------------
188-
| Set parameters
190+
| Handle AttributeDictionary constructor
191+
>-------------------------------------------------------------------------------------------------------------------------
192+
| A model may optionally expose a constructor with a single parameter accepting an AttributeDictionary. In this scenario,
193+
| the TopicMappingService may optionally pass a lightweight AttributeDictionary, allowing the model's constructor to
194+
| populate scalar values, instead of relying on reflection.
189195
\-----------------------------------------------------------------------------------------------------------------------*/
190-
var parameterQueue = new Dictionary<int, Task<object?>>();
191-
192196
if (parameters.Count is 1 && parameters[0].Type == typeof(AttributeDictionary)) {
193-
var attributes = topic.Attributes.AsAttributeDictionary(true);
194-
arguments[0] = attributes;
195-
attributeArguments = attributes;
197+
198+
// This strategy is only performant if there are quite a several scalar properties and they are well-covered by the
199+
// attributes. As a fast heuristic to evaluate this, we expect five or more attributes and properties. In practice, this
200+
// should be benefitial with any more than mapped attributes, but we also expect that most topics will have 2-3 excluded
201+
// or unmapped attributes (e.g., Title, LastModified) and that models will have five or properties that aren't mapped to
202+
// attributes (e.g., Id, Key, WebPath). This doesn't guarantee that the attributes map to the properties, but a more
203+
// accurate evaluation undermines the performance benefits of this optimization.
204+
if (topic.Attributes.Count >= 5 && properties.Count(p => p.IsConvertible) >= 5) {
205+
var attributes = topic.Attributes.AsAttributeDictionary(true);
206+
arguments[0] = attributes;
207+
attributeArguments = attributes;
208+
}
209+
else {
210+
parameters = new();
211+
arguments = Array.Empty<object?>();
212+
}
213+
196214
}
215+
216+
/*------------------------------------------------------------------------------------------------------------------------
217+
| Handle other constructors
218+
>-------------------------------------------------------------------------------------------------------------------------
219+
| A model may optionally expose a constructor with multiple parameters, which can be defined via reflection in the same
220+
| way as properties would be. This is especially useful for records using the positional syntax (i.e., where properties
221+
| are defined using the constructor). This also, optionally, provides the model with more control, where needed, over how
222+
| it's constructed.
223+
\-----------------------------------------------------------------------------------------------------------------------*/
197224
else {
198225

199226
foreach (var parameter in parameters) {

0 commit comments

Comments
 (0)