From 99b89ae2765c40b91abd4fc761e2c04ce412070d Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 16 Apr 2024 21:19:51 -0600 Subject: [PATCH 1/2] Remove slice! from fixed_protocol --- .../interfaces/protocols/fixed_protocol.rb | 12 +++-- .../protocols/fixed_protocol_spec.rb | 41 +++++++++++++++ .../test/benchmarks/string_mod_benchmark.rb | 50 +++++++++++++++++++ openc3/test/benchmarks/system_benchmark.rb | 18 +++++++ 4 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 openc3/test/benchmarks/string_mod_benchmark.rb diff --git a/openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb b/openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb index 4958cf8aa8..a5032f6aa6 100644 --- a/openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb +++ b/openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb @@ -102,7 +102,7 @@ def identify_and_finish_packet end if unique_id_mode - target_packets.each do |packet_name, packet| + target_packets.each do |_packet_name, packet| if packet.identify?(@data[@discard_leading_bytes..-1]) identified_packet = packet break @@ -136,7 +136,13 @@ def identify_and_finish_packet @packet_name = identified_packet.packet_name # Get the data from this packet - packet_data = @data.slice!(0, identified_packet.defined_length + @discard_leading_bytes) + # Previous implementation looked like the following: + # packet_data = @data.slice!(0, identified_packet.defined_length + @discard_leading_bytes) + # But slice! is 6x slower at small packets (1024) + # and 1000x slower at large packets (1Mb) + # Run test/benchmarks/string_mod_benchmark.rb for details + packet_data = @data[0...identified_packet.defined_length + @discard_leading_bytes] + @data = @data[(identified_packet.defined_length + @discard_leading_bytes)..-1] break end end @@ -152,7 +158,7 @@ def identify_and_finish_packet @data.replace('') end - return packet_data + return packet_data, @extra end def reduce_to_single_packet diff --git a/openc3/spec/interfaces/protocols/fixed_protocol_spec.rb b/openc3/spec/interfaces/protocols/fixed_protocol_spec.rb index 13d5ba2438..68b3bcd3c0 100644 --- a/openc3/spec/interfaces/protocols/fixed_protocol_spec.rb +++ b/openc3/spec/interfaces/protocols/fixed_protocol_spec.rb @@ -179,6 +179,47 @@ def read expect(packet.buffer).to eql $buffer target.cmd_unique_id_mode = false end + + it "breaks apart telemetry data from the stream" do + packet = System.telemetry.packet("SYSTEM", "META") + packet.write('PKTID', 1) + packet.write('OPERATOR_NAME', 'RYAN') + $buffer1 = packet.buffer.clone + packet.write('OPERATOR_NAME', 'JASON') + $buffer2 = packet.buffer.clone + $buffer = "\x1A\xCF\xFC\x1D" + $buffer1 + "\x1A\xCF\xFC\x1D" + $buffer2 + $index = 0 + + class FixedStream3 < Stream + def connect; end + + def connected?; true; end + + def read + # Send a byte a time + $index += 1 + $buffer[$index - 1] + end + end + # Require 5 bytes, discard 4 leading bytes, use 0x1ACFFC1D sync, telemetry = true + @interface.add_protocol(FixedProtocol, [5, 4, '0x1ACFFC1D', true], :READ_WRITE) + @interface.instance_variable_set(:@stream, FixedStream3.new) + @interface.target_names = ['SYSTEM'] + @interface.cmd_target_names = ['SYSTEM'] + @interface.tlm_target_names = ['SYSTEM'] + packet = @interface.read + expect(packet.received_time.to_f).to be_within(0.01).of(Time.now.to_f) + expect(packet.target_name).to eql 'SYSTEM' + expect(packet.packet_name).to eql 'META' + expect(packet.buffer).to include('RYAN') + packet = @interface.read + expect(packet.received_time.to_f).to be_within(0.01).of(Time.now.to_f) + expect(packet.target_name).to eql 'SYSTEM' + expect(packet.packet_name).to eql 'META' + expect(packet.buffer).to include('JASON') + packet = @interface.read + expect(packet).to be_nil + end end end end diff --git a/openc3/test/benchmarks/string_mod_benchmark.rb b/openc3/test/benchmarks/string_mod_benchmark.rb new file mode 100644 index 0000000000..f20c6ca90d --- /dev/null +++ b/openc3/test/benchmarks/string_mod_benchmark.rb @@ -0,0 +1,50 @@ +# encoding: ascii-8bit + +# Copyright 2023 OpenC3, Inc. +# All Rights Reserved. +# +# This program is free software; you can modify and/or redistribute it +# under the terms of the GNU Affero General Public License +# as published by the Free Software Foundation; version 3 with +# attribution addendums as found in the LICENSE.txt +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# This file may also be used under the terms of a commercial license +# if purchased from OpenC3, Inc. + +require 'securerandom' +require 'benchmark' + +Benchmark.bm do |x| + data = SecureRandom.random_bytes(1024) + x.report("slice!") do + 1000.times.each { data.slice!(0, 1) } + end + data = SecureRandom.random_bytes(1024) + x.report("assign") do + 1000.times.each { data = data[1..-1] } + end + data = SecureRandom.random_bytes(1024) + x.report("replace") do + 1000.times.each { data.replace(data[1..-1]) } + end +end + +Benchmark.bm do |x| + data = SecureRandom.random_bytes(1024*1024) + x.report("slice!") do + 1000.times.each { data.slice!(0, 1) } + end + data = SecureRandom.random_bytes(1024*1024) + x.report("assign") do + 1000.times.each { data = data[1..-1] } + end + data = SecureRandom.random_bytes(1024*1024) + x.report("replace") do + 1000.times.each { data.replace(data[1..-1]) } + end +end diff --git a/openc3/test/benchmarks/system_benchmark.rb b/openc3/test/benchmarks/system_benchmark.rb index 3f1687ad84..6ff224fba0 100644 --- a/openc3/test/benchmarks/system_benchmark.rb +++ b/openc3/test/benchmarks/system_benchmark.rb @@ -1,3 +1,21 @@ +# encoding: ascii-8bit + +# Copyright 2023 OpenC3, Inc. +# All Rights Reserved. +# +# This program is free software; you can modify and/or redistribute it +# under the terms of the GNU Affero General Public License +# as published by the Free Software Foundation; version 3 with +# attribution addendums as found in the LICENSE.txt +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# This file may also be used under the terms of a commercial license +# if purchased from OpenC3, Inc. + require 'openc3' require 'benchmark' ENV['OPENC3_NO_STORE'] = 'true' From f96ef0bee49bb6403593236bbddd125126e77408 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 29 Apr 2024 08:57:18 -0600 Subject: [PATCH 2/2] Clarify change --- openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb b/openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb index a5032f6aa6..803ac7ca3b 100644 --- a/openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb +++ b/openc3/lib/openc3/interfaces/protocols/fixed_protocol.rb @@ -141,7 +141,9 @@ def identify_and_finish_packet # But slice! is 6x slower at small packets (1024) # and 1000x slower at large packets (1Mb) # Run test/benchmarks/string_mod_benchmark.rb for details - packet_data = @data[0...identified_packet.defined_length + @discard_leading_bytes] + + # Triple dot range because it's effectively a length calculation and we start with 0 + packet_data = @data[0...(identified_packet.defined_length + @discard_leading_bytes)] @data = @data[(identified_packet.defined_length + @discard_leading_bytes)..-1] break end