Skip to content

Commit 41dda40

Browse files
ioquatixioquatix
authored
Improve performance of dependency management - avoid linear search. (#22)
1 parent 191c31f commit 41dda40

File tree

2 files changed

+223
-69
lines changed

2 files changed

+223
-69
lines changed

‎lib/protocol/http2/dependency.rb

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ def <=> other
5858
@weight <=> other.weight
5959
end
6060

61+
def == other
62+
@id == other.id
63+
end
64+
6165
# The connection this stream belongs to.
6266
attr :connection
6367

@@ -101,13 +105,30 @@ def add_child(dependency)
101105

102106
dependency.parent = self
103107

104-
self.clear_cache!
108+
if @ordered_children
109+
# Binary search for insertion point:
110+
index = @ordered_children.bsearch_index do |child|
111+
child.weight >= dependency.weight
112+
end
113+
114+
if index
115+
@ordered_children.insert(index, dependency)
116+
else
117+
@ordered_children.push(dependency)
118+
end
119+
120+
@total_weight += dependency.weight
121+
end
105122
end
106123

107124
def remove_child(dependency)
108125
@children&.delete(dependency.id)
109126

110-
self.clear_cache!
127+
if @ordered_children
128+
# Don't do a linear search here, it can be slow. Instead, the child's parent will be set to `nil`, and we check this in {#consume_window} using `delete_if`.
129+
# @ordered_children.delete(dependency)
130+
@total_weight -= dependency.weight
131+
end
111132
end
112133

113134
# An exclusive flag allows for the insertion of a new level of dependencies. The exclusive flag causes the stream to become the sole dependency of its parent stream, causing other dependencies to become dependent on the exclusive stream.
@@ -195,11 +216,17 @@ def consume_window(size)
195216
end
196217

197218
# Otherwise, allow the dependent children to use up the available window:
198-
self.ordered_children&.each do |child|
199-
# Compute the proportional allocation:
200-
allocated = (child.weight * size) / @total_weight
201-
202-
child.consume_window(allocated) if allocated > 0
219+
self.ordered_children&.delete_if do |child|
220+
if child.parent
221+
# Compute the proportional allocation:
222+
allocated = (child.weight * size) / @total_weight
223+
224+
child.consume_window(allocated) if allocated > 0
225+
226+
false
227+
else
228+
true
229+
end
203230
end
204231
end
205232

‎test/protocol/http2/dependency.rb

Lines changed: 189 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -48,16 +48,14 @@ def before
4848
# a
4949
# /
5050
# b
51-
begin
52-
a.priority = a.priority
53-
server.read_frame
54-
55-
priority = b.priority
56-
priority.stream_dependency = a.id
57-
b.priority = priority
58-
59-
server.read_frame
60-
end
51+
a.priority = a.priority
52+
server.read_frame
53+
54+
priority = b.priority
55+
priority.stream_dependency = a.id
56+
b.priority = priority
57+
58+
server.read_frame
6159

6260
expect(a.dependency.children).to be == {b.id => b.dependency}
6361

@@ -68,13 +66,11 @@ def before
6866
# a
6967
# / \
7068
# b c
71-
begin
72-
priority = c.priority
73-
priority.stream_dependency = a.id
74-
c.priority = priority
75-
76-
server.read_frame
77-
end
69+
priority = c.priority
70+
priority.stream_dependency = a.id
71+
c.priority = priority
72+
73+
server.read_frame
7874

7975
expect(a.dependency.children).to be == {b.id => b.dependency, c.id => c.dependency}
8076
expect(server.dependencies[a.id].children).to be == {b.id => server.dependencies[b.id], c.id => server.dependencies[c.id]}
@@ -84,14 +80,12 @@ def before
8480
# d
8581
# / \
8682
# b c
87-
begin
88-
priority = d.priority
89-
priority.stream_dependency = a.id
90-
priority.exclusive = true
91-
d.priority = priority
92-
93-
server.read_frame
94-
end
83+
priority = d.priority
84+
priority.stream_dependency = a.id
85+
priority.exclusive = true
86+
d.priority = priority
87+
88+
server.read_frame
9589

9690
expect(a.dependency.children).to be == {d.id => d.dependency}
9791
expect(d.dependency.children).to be == {b.id => b.dependency, c.id => c.dependency}
@@ -163,36 +157,32 @@ def before
163157
# b c
164158
# |
165159
# d
166-
begin
167-
a.priority = a.priority
168-
server.read_frame
169-
170-
priority = b.priority
171-
priority.stream_dependency = a.id
172-
b.priority = priority
173-
server.read_frame
174-
175-
priority = c.priority
176-
priority.stream_dependency = a.id
177-
c.priority = priority
178-
server.read_frame
179-
180-
priority = d.priority
181-
priority.stream_dependency = c.id
182-
d.priority = priority
183-
server.read_frame
184-
end
160+
a.priority = a.priority
161+
server.read_frame
162+
163+
priority = b.priority
164+
priority.stream_dependency = a.id
165+
b.priority = priority
166+
server.read_frame
167+
168+
priority = c.priority
169+
priority.stream_dependency = a.id
170+
c.priority = priority
171+
server.read_frame
172+
173+
priority = d.priority
174+
priority.stream_dependency = c.id
175+
d.priority = priority
176+
server.read_frame
185177

186178
expect(server.dependencies[a.id].children).to be == {b.id => server.dependencies[b.id], c.id => server.dependencies[c.id]}
187179
expect(server.dependencies[c.id].children).to be == {d.id => server.dependencies[d.id]}
188180

189181
# a
190182
# / \
191183
# b d
192-
begin
193-
c.send_reset_stream
194-
server.read_frame
195-
end
184+
c.send_reset_stream
185+
server.read_frame
196186

197187
expect(server.dependencies[a.id].children).to be == {b.id => server.dependencies[b.id], d.id => server.dependencies[d.id]}
198188
expect(server.dependencies[c.id]).to be == nil
@@ -205,20 +195,18 @@ def before
205195
# a
206196
# / \
207197
# b c
208-
begin
209-
a.priority = a.priority
210-
server.read_frame
211-
212-
priority = b.priority
213-
priority.stream_dependency = a.id
214-
b.priority = priority
215-
server.read_frame
216-
217-
priority = c.priority
218-
priority.stream_dependency = a.id
219-
c.priority = priority
220-
server.read_frame
221-
end
198+
a.priority = a.priority
199+
server.read_frame
200+
201+
priority = b.priority
202+
priority.stream_dependency = a.id
203+
b.priority = priority
204+
server.read_frame
205+
206+
priority = c.priority
207+
priority.stream_dependency = a.id
208+
c.priority = priority
209+
server.read_frame
222210

223211
a.dependency.print_hierarchy(buffer)
224212

@@ -228,4 +216,143 @@ def before
228216
#<Protocol::HTTP2::Dependency id=5 parent id=1 weight=16 0 children>
229217
END
230218
end
219+
220+
it "can insert several children" do
221+
a, b, c, d = 4.times.collect {client.create_stream}
222+
223+
b.dependency.weight = 10
224+
c.dependency.weight = 20
225+
d.dependency.weight = 30
226+
227+
# a
228+
# / | \
229+
# b c d
230+
231+
a.priority = a.priority
232+
server.read_frame
233+
234+
priority = d.priority
235+
priority.stream_dependency = a.id
236+
d.priority = priority
237+
server.read_frame
238+
239+
# Force the ordered_children to be generated, so that we start triggering insertion logic:
240+
ordered_children = server.dependencies[a.id].ordered_children
241+
242+
priority = b.priority
243+
priority.stream_dependency = a.id
244+
b.priority = priority
245+
server.read_frame
246+
247+
priority = c.priority
248+
priority.stream_dependency = a.id
249+
c.priority = priority
250+
server.read_frame
251+
252+
expect(ordered_children).to be == [b.dependency, c.dependency, d.dependency]
253+
end
254+
255+
it "handles case where index is nil during insertion" do
256+
a, b, c, d = 4.times.collect { client.create_stream }
257+
258+
# Assign weights such that no existing child meets the condition:
259+
b.dependency.weight = 5
260+
c.dependency.weight = 10
261+
d.dependency.weight = 15
262+
263+
# Priority tree setup:
264+
# a
265+
# /|\
266+
# b c d
267+
268+
a.priority = a.priority
269+
server.read_frame
270+
271+
priority = b.priority
272+
priority.stream_dependency = a.id
273+
b.priority = priority
274+
server.read_frame
275+
276+
# Force the ordered_children to be generated, so that we start triggering insertion logic:
277+
ordered_children = server.dependencies[a.id].ordered_children
278+
279+
priority = c.priority
280+
priority.stream_dependency = a.id
281+
c.priority = priority
282+
server.read_frame
283+
284+
priority = d.priority
285+
priority.stream_dependency = a.id
286+
d.priority = priority
287+
server.read_frame
288+
289+
# Insert a new dependency with a weight that is higher than all current children:
290+
new_child = client.create_stream
291+
new_child.dependency.weight = 20 # Greater than the weights of b, c, and d
292+
293+
# Verify that `index` would have been `nil` when finding the insertion point:
294+
index = ordered_children.bsearch_index { |child| child.weight >= new_child.dependency.weight }
295+
expect(index).to be_nil
296+
297+
priority = new_child.priority
298+
priority.stream_dependency = a.id
299+
new_child.priority = priority
300+
server.read_frame
301+
302+
# Check that the new child is inserted at the correct place:
303+
expect(ordered_children).to be(:include?, new_child.dependency)
304+
expect(ordered_children.last).to be == new_child.dependency
305+
end
306+
307+
it "can update the weight of a child on removal" do
308+
a, b, c, d = 4.times.collect { client.create_stream }
309+
310+
# Assign weights such that no existing child meets the condition:
311+
b.dependency.weight = 5
312+
c.dependency.weight = 10
313+
d.dependency.weight = 15
314+
315+
# Priority tree setup:
316+
# a
317+
# /|\
318+
# b c d
319+
320+
a.priority = a.priority
321+
server.read_frame
322+
323+
priority = b.priority
324+
priority.stream_dependency = a.id
325+
b.priority = priority
326+
server.read_frame
327+
328+
# Force the ordered_children to be generated, so that we start triggering insertion logic:
329+
ordered_children = server.dependencies[a.id].ordered_children
330+
331+
priority = c.priority
332+
priority.stream_dependency = a.id
333+
c.priority = priority
334+
server.read_frame
335+
336+
priority = d.priority
337+
priority.stream_dependency = a.id
338+
d.priority = priority
339+
server.read_frame
340+
341+
expect(ordered_children).to be == [b.dependency, c.dependency, d.dependency]
342+
343+
# Check the total weight makes sense:
344+
expect(server.dependencies[a.id].total_weight).to be == 30
345+
346+
# Remove the child with the highest weight:
347+
server.delete(d.id)
348+
349+
# Check that the weight of the remaining children has been updated:
350+
expect(server.dependencies[a.id].total_weight).to be == 15
351+
352+
# Force the deletion logic to clean up `ordered_children`:
353+
server.dependencies[a.id].consume_window(15)
354+
355+
# Check that the child has been removed:
356+
expect(ordered_children).to be == [b.dependency, c.dependency]
357+
end
231358
end

0 commit comments

Comments
 (0)